PyType::py_new to return Self #6398
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWidespread constructor API overhaul: introduce a slot_new/py_new contract where py_new takes &Py and returns PyResult (often unused), many constructors moved to slot_new(FuncArgs) with args.bind(vm), and object creation now returns owned Self directly instead of into_ref_with_type/.into(). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
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 selected for processing (3)
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 |
|
Very nice! I like this idea a lot |
551c683 to
9462d44
Compare
There was a problem hiding this comment.
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/stdlib/ctypes/structure.rs (1)
243-337:PyCStructure::slot_newshould avoid swallowing non‑AttributeErrorexceptions from_fields_The overall shape of
slot_new(derivefields_mapfrom_fields_, allocateCDataObjectviaStgInfo, then assign field values fromargs.args/args.kwargs) looks sound and matches ctypes semantics at a high level. One concern is error handling around_fields_:let fields_attr = cls.as_object().get_attr("_fields_", vm).ok();This treats any exception raised while resolving
_fields_(including arbitrary user errors from descriptors/properties) as “no_fields_defined”, silently continuing with an empty layout. That can make debugging hard and diverges from the expectation that such errors propagate.Consider only suppressing
AttributeErrorhere and propagating all other exceptions, e.g.:- let fields_attr = cls.as_object().get_attr("_fields_", vm).ok(); + let fields_attr = match cls.as_object().get_attr("_fields_", vm) { + Ok(obj) => Some(obj), + Err(e) if e.fast_isinstance(vm.ctx.exceptions.attribute_error) => None, + Err(e) => return Err(e), + };As a future improvement, you might also cache the parsed field layout (size/offsets) per type (e.g., in
StgInfoonPyCStructType) rather than recomputing it on every instance creation, but that’s an optimization rather than a correctness fix.
🧹 Nitpick comments (5)
crates/vm/src/exceptions.rs (1)
719-721: Consider usingunimplemented!()instead ofunreachable!()for the placeholder.The
unreachable!()macro is semantically misleading here since this code path is technically reachable ifpy_newis called directly. Theunimplemented!()macro more accurately conveys that this is an intentional placeholder during the transitional refactoring period.Apply this diff:
- fn py_new(_cls: &Py<PyType>, _args: FuncArgs, _vm: &VirtualMachine) -> PyResult<Self> { - unreachable!("use slot_new") - } + fn py_new(_cls: &Py<PyType>, _args: FuncArgs, _vm: &VirtualMachine) -> PyResult<Self> { + unimplemented!("use slot_new") + }crates/vm/src/builtins/classmethod.rs (1)
110-117: Minor: Initializer doesn't update dict attributes.The
initmethod updatesself.callablebut doesn't refresh the dict attributes (__doc__,__name__, etc.) that were copied inslot_new. If someone creates a classmethod and later reinitializes it with a different callable via__init__, the dict would retain stale metadata from the original callable.This is likely an edge case matching CPython behavior, but worth noting.
crates/vm/src/stdlib/ctypes/structure.rs (1)
25-49:PyCStructType::slot_newcorrectly piggybacks ontype.__new__and hooks_fields_Delegating to
PyType::slot_newand then processing_fields_only when defined directly on the new class is a clean way to layer ctypes-specific behavior over the core metatype, and the unreachablepy_newmakes it clear that only the slot path is valid.You now have
_fields_processing both here and in__init_subclass__; if both are intended, the duplication is mostly harmless (replacing descriptors), but if not, consider consolidating into one place to avoid surprises.crates/vm/src/stdlib/ctypes/base.rs (1)
780-803: ReusingPyCSimple::slot_newinfrom_address/from_buffer/from_buffer_copy/in_dllcentralizes initializationConstructing a single-argument
FuncArgs(FuncArgs::new(vec![value], KwArgs::default())) and delegating toPyCSimple::slot_new(cls.clone(), args, vm)makes these helpers share the same validation/defaulting and endianness logic as regular construction. This refactor reduces duplication without introducing obvious behavior changes; the existing TODOs about lifetime tracking (_objects) remain valid but are unaffected.Also applies to: 810-903, 906-971
crates/vm/src/stdlib/ctypes/union.rs (1)
32-34: Stale comment referencespy_newinstead ofslot_new.The comment on line 33 says "Create the new class using PyType::py_new" but the actual call on line 34 is to
PyType::slot_new. This should be updated for consistency.fn slot_new(metatype: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult { - // 1. Create the new class using PyType::py_new + // 1. Create the new class using PyType::slot_new let new_class = crate::builtins::type_::PyType::slot_new(metatype, args, vm)?;
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
crates/stdlib/src/array.rs(3 hunks)crates/stdlib/src/bz2.rs(4 hunks)crates/stdlib/src/contextvars.rs(4 hunks)crates/stdlib/src/csv.rs(1 hunks)crates/stdlib/src/json.rs(3 hunks)crates/stdlib/src/lzma.rs(5 hunks)crates/stdlib/src/mmap.rs(6 hunks)crates/stdlib/src/overlapped.rs(3 hunks)crates/stdlib/src/pystruct.rs(2 hunks)crates/stdlib/src/select.rs(2 hunks)crates/stdlib/src/sqlite.rs(6 hunks)crates/stdlib/src/ssl.rs(6 hunks)crates/stdlib/src/zlib.rs(3 hunks)crates/vm/src/builtins/bool.rs(3 hunks)crates/vm/src/builtins/bytes.rs(2 hunks)crates/vm/src/builtins/classmethod.rs(3 hunks)crates/vm/src/builtins/code.rs(3 hunks)crates/vm/src/builtins/complex.rs(3 hunks)crates/vm/src/builtins/enumerate.rs(1 hunks)crates/vm/src/builtins/filter.rs(1 hunks)crates/vm/src/builtins/float.rs(3 hunks)crates/vm/src/builtins/function.rs(5 hunks)crates/vm/src/builtins/genericalias.rs(2 hunks)crates/vm/src/builtins/int.rs(3 hunks)crates/vm/src/builtins/list.rs(1 hunks)crates/vm/src/builtins/map.rs(1 hunks)crates/vm/src/builtins/mappingproxy.rs(1 hunks)crates/vm/src/builtins/memory.rs(1 hunks)crates/vm/src/builtins/object.rs(2 hunks)crates/vm/src/builtins/property.rs(3 hunks)crates/vm/src/builtins/set.rs(3 hunks)crates/vm/src/builtins/singletons.rs(3 hunks)crates/vm/src/builtins/slice.rs(1 hunks)crates/vm/src/builtins/staticmethod.rs(2 hunks)crates/vm/src/builtins/str.rs(2 hunks)crates/vm/src/builtins/super.rs(1 hunks)crates/vm/src/builtins/traceback.rs(2 hunks)crates/vm/src/builtins/tuple.rs(3 hunks)crates/vm/src/builtins/type.rs(2 hunks)crates/vm/src/builtins/weakproxy.rs(2 hunks)crates/vm/src/builtins/weakref.rs(2 hunks)crates/vm/src/builtins/zip.rs(2 hunks)crates/vm/src/exceptions.rs(1 hunks)crates/vm/src/protocol/object.rs(3 hunks)crates/vm/src/stdlib/ast/python.rs(2 hunks)crates/vm/src/stdlib/collections.rs(3 hunks)crates/vm/src/stdlib/ctypes/array.rs(4 hunks)crates/vm/src/stdlib/ctypes/base.rs(7 hunks)crates/vm/src/stdlib/ctypes/function.rs(5 hunks)crates/vm/src/stdlib/ctypes/pointer.rs(3 hunks)crates/vm/src/stdlib/ctypes/structure.rs(4 hunks)crates/vm/src/stdlib/ctypes/union.rs(4 hunks)crates/vm/src/stdlib/functools.rs(2 hunks)crates/vm/src/stdlib/io.rs(2 hunks)crates/vm/src/stdlib/itertools.rs(26 hunks)crates/vm/src/stdlib/operator.rs(5 hunks)crates/vm/src/stdlib/posix.rs(2 hunks)crates/vm/src/stdlib/thread.rs(2 hunks)crates/vm/src/stdlib/typevar.rs(10 hunks)crates/vm/src/stdlib/typing.rs(4 hunks)crates/vm/src/types/slot.rs(2 hunks)
🧰 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/float.rscrates/vm/src/builtins/mappingproxy.rscrates/vm/src/exceptions.rscrates/stdlib/src/lzma.rscrates/vm/src/builtins/str.rscrates/vm/src/builtins/memory.rscrates/vm/src/builtins/traceback.rscrates/stdlib/src/ssl.rscrates/vm/src/protocol/object.rscrates/vm/src/builtins/tuple.rscrates/vm/src/builtins/super.rscrates/stdlib/src/json.rscrates/vm/src/builtins/filter.rscrates/vm/src/builtins/staticmethod.rscrates/vm/src/builtins/weakproxy.rscrates/stdlib/src/select.rscrates/vm/src/builtins/type.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/builtins/genericalias.rscrates/vm/src/builtins/weakref.rscrates/vm/src/stdlib/operator.rscrates/vm/src/stdlib/thread.rscrates/vm/src/builtins/singletons.rscrates/stdlib/src/mmap.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/complex.rscrates/stdlib/src/csv.rscrates/vm/src/stdlib/io.rscrates/vm/src/builtins/set.rscrates/vm/src/builtins/function.rscrates/stdlib/src/pystruct.rscrates/vm/src/builtins/zip.rscrates/vm/src/stdlib/collections.rscrates/vm/src/stdlib/functools.rscrates/vm/src/stdlib/ctypes/function.rscrates/vm/src/builtins/code.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/stdlib/src/bz2.rscrates/vm/src/builtins/bytes.rscrates/vm/src/stdlib/posix.rscrates/vm/src/stdlib/ctypes/structure.rscrates/vm/src/stdlib/ctypes/union.rscrates/vm/src/builtins/slice.rscrates/vm/src/stdlib/typevar.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/int.rscrates/stdlib/src/contextvars.rscrates/vm/src/builtins/map.rscrates/vm/src/builtins/bool.rscrates/stdlib/src/array.rscrates/vm/src/stdlib/typing.rscrates/vm/src/stdlib/ast/python.rscrates/vm/src/builtins/classmethod.rscrates/vm/src/stdlib/ctypes/array.rscrates/stdlib/src/sqlite.rscrates/vm/src/builtins/object.rscrates/stdlib/src/zlib.rscrates/vm/src/builtins/enumerate.rscrates/stdlib/src/overlapped.rscrates/vm/src/builtins/property.rscrates/vm/src/stdlib/itertools.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/vm/src/exceptions.rscrates/stdlib/src/lzma.rscrates/stdlib/src/overlapped.rscrates/vm/src/stdlib/itertools.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/stdlib/src/lzma.rscrates/vm/src/builtins/traceback.rscrates/stdlib/src/ssl.rscrates/vm/src/protocol/object.rscrates/vm/src/builtins/tuple.rscrates/stdlib/src/json.rscrates/vm/src/builtins/weakref.rscrates/vm/src/builtins/singletons.rscrates/vm/src/builtins/function.rscrates/stdlib/src/pystruct.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/stdlib/typevar.rscrates/vm/src/stdlib/typing.rscrates/vm/src/stdlib/ast/python.rscrates/vm/src/builtins/classmethod.rs
🧬 Code graph analysis (42)
crates/vm/src/builtins/float.rs (2)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/slice.rs (2)
slot_new(67-91)slot_new(316-319)
crates/vm/src/exceptions.rs (2)
crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/exception_group.rs (1)
slot_new(63-160)
crates/stdlib/src/lzma.rs (3)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/vm/src/builtins/str.rs (2)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/int.rs (1)
slot_new(210-246)
crates/vm/src/builtins/memory.rs (3)
crates/stdlib/src/array.rs (1)
py_new(654-706)crates/stdlib/src/sqlite.rs (3)
py_new(854-881)args(422-426)args(619-623)crates/vm/src/stdlib/ctypes/function.rs (2)
args(302-302)args(366-376)
crates/vm/src/builtins/traceback.rs (1)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)
crates/stdlib/src/ssl.rs (4)
crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/builtins/super.rs (6)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/stdlib/src/sqlite.rs (4)
py_new(854-881)new(396-401)new(1543-1561)new(2597-2644)crates/vm/src/builtins/code.rs (3)
new(29-37)new(335-337)new(1035-1037)
crates/stdlib/src/json.rs (3)
crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/lzma.rs (2)
py_new(151-171)py_new(367-394)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/vm/src/builtins/filter.rs (5)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/mmap.rs (2)
py_new(391-490)py_new(493-638)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/stdlib/src/sqlite.rs (1)
py_new(854-881)
crates/vm/src/builtins/staticmethod.rs (2)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/classmethod.rs (1)
slot_new(73-103)
crates/stdlib/src/select.rs (4)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/lzma.rs (3)
py_new(151-171)py_new(367-394)new(259-261)crates/stdlib/src/ssl.rs (1)
e(1242-1242)
crates/vm/src/builtins/type.rs (4)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (2)
slot_new(35-111)py_new(113-115)crates/vm/src/stdlib/typevar.rs (10)
slot_new(264-270)slot_new(539-610)slot_new(725-772)args(192-192)args(427-433)py_new(272-376)py_new(612-614)py_new(774-776)py_new(819-822)py_new(897-900)
crates/vm/src/stdlib/ctypes/base.rs (5)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/stdlib/ctypes/array.rs (8)
slot_new(252-305)py_new(78-80)py_new(307-309)value(462-466)value(470-470)value(487-487)instance(197-197)instance(746-746)crates/vm/src/stdlib/ctypes/function.rs (5)
slot_new(189-330)args(302-302)args(366-376)py_new(332-334)value(52-52)crates/vm/src/stdlib/ctypes/pointer.rs (2)
slot_new(79-90)py_new(92-94)crates/vm/src/stdlib/ctypes/structure.rs (4)
slot_new(28-44)slot_new(246-332)py_new(46-48)py_new(334-336)
crates/vm/src/builtins/genericalias.rs (2)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)
crates/vm/src/builtins/weakref.rs (5)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/complex.rs (1)
slot_new(154-228)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/stdlib/operator.rs (1)
crates/vm/src/builtins/code.rs (5)
args(395-404)args(407-416)args(419-428)args(431-440)args(458-466)
crates/vm/src/stdlib/thread.rs (5)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/lzma.rs (2)
py_new(151-171)py_new(367-394)crates/stdlib/src/sqlite.rs (1)
py_new(854-881)
crates/vm/src/builtins/singletons.rs (5)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/float.rs (1)
slot_new(134-153)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)crates/vm/src/builtins/tuple.rs (1)
slot_new(115-140)
crates/stdlib/src/csv.rs (2)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/json.rs (1)
py_new(36-64)
crates/vm/src/stdlib/io.rs (2)
crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)
crates/vm/src/builtins/set.rs (4)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/str.rs (1)
slot_new(353-379)
crates/vm/src/builtins/function.rs (10)
crates/stdlib/src/array.rs (2)
py_new(654-706)value(1016-1016)crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/stdlib/src/sqlite.rs (8)
py_new(854-881)args(422-426)args(619-623)new(396-401)new(1543-1561)new(2597-2644)value(2425-2425)value(2431-2431)crates/vm/src/builtins/code.rs (8)
args(395-404)args(407-416)args(419-428)args(431-440)args(458-466)new(29-37)new(335-337)new(1035-1037)crates/vm/src/stdlib/ctypes/function.rs (2)
args(302-302)args(366-376)crates/vm/src/stdlib/io.rs (1)
vm(3478-3480)crates/vm/src/builtins/staticmethod.rs (1)
new(69-73)crates/vm/src/stdlib/ctypes/base.rs (1)
value(54-56)
crates/stdlib/src/pystruct.rs (2)
crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/sqlite.rs (2)
py_new(854-881)py_new(1941-1947)
crates/vm/src/stdlib/functools.rs (3)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)
crates/vm/src/stdlib/ctypes/function.rs (2)
crates/vm/src/stdlib/ctypes/base.rs (2)
slot_new(613-662)py_new(664-666)crates/vm/src/stdlib/ctypes/pointer.rs (2)
slot_new(79-90)py_new(92-94)
crates/vm/src/stdlib/ctypes/pointer.rs (3)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/float.rs (1)
slot_new(134-153)crates/vm/src/stdlib/ctypes/base.rs (2)
slot_new(613-662)py_new(664-666)
crates/stdlib/src/bz2.rs (3)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/vm/src/builtins/bytes.rs (4)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/singletons.rs (2)
slot_new(42-45)slot_new(96-99)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)crates/vm/src/builtins/str.rs (1)
slot_new(353-379)
crates/vm/src/stdlib/ctypes/union.rs (1)
crates/vm/src/stdlib/ctypes/base.rs (2)
slot_new(613-662)py_new(664-666)
crates/vm/src/stdlib/typevar.rs (2)
crates/vm/src/builtins/object.rs (2)
slot_new(35-111)py_new(113-115)crates/vm/src/builtins/type.rs (5)
slot_new(994-1289)py_new(1291-1293)obj(216-216)obj(577-577)obj(1027-1027)
crates/vm/src/types/slot.rs (1)
crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/int.rs (3)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/map.rs (8)
crates/stdlib/src/array.rs (1)
py_new(654-706)crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/lzma.rs (2)
py_new(151-171)py_new(367-394)crates/stdlib/src/overlapped.rs (1)
py_new(267-299)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/stdlib/src/sqlite.rs (1)
py_new(854-881)
crates/vm/src/builtins/bool.rs (2)
crates/vm/src/builtins/float.rs (1)
slot_new(134-153)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/stdlib/src/array.rs (3)
crates/vm/src/builtins/bytes.rs (2)
from(44-48)from(52-54)crates/vm/src/builtins/classmethod.rs (1)
from(37-41)crates/vm/src/builtins/list.rs (1)
from(39-43)
crates/vm/src/stdlib/ast/python.rs (5)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/mmap.rs (2)
py_new(391-490)py_new(493-638)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/stdlib/src/sqlite.rs (1)
py_new(854-881)
crates/vm/src/builtins/classmethod.rs (3)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/builtins/object.rs (1)
crates/vm/src/builtins/type.rs (2)
slot_new(994-1289)py_new(1291-1293)
crates/stdlib/src/zlib.rs (3)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/lzma.rs (3)
py_new(151-171)py_new(367-394)new(259-261)
crates/stdlib/src/overlapped.rs (7)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/vm/src/stdlib/posix.rs (1)
py_new(835-839)crates/vm/src/builtins/enumerate.rs (1)
new(102-107)
crates/vm/src/builtins/property.rs (3)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-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). (9)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
| impl Constructor for PyCArrayType { | ||
| type Args = PyObjectRef; | ||
|
|
||
| fn py_new(_cls: PyTypeRef, _args: Self::Args, _vm: &VirtualMachine) -> PyResult { | ||
| unreachable!() | ||
| fn py_new(_cls: &Py<PyType>, _args: Self::Args, _vm: &VirtualMachine) -> PyResult<Self> { | ||
| unreachable!("use slot_new") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for slot_new in array.rs
rg -n "slot_new" crates/vm/src/stdlib/ctypes/array.rsRepository: RustPython/RustPython
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check the full PyCArrayType impl block and surrounding context
rg -B5 -A20 "impl Constructor for PyCArrayType" crates/vm/src/stdlib/ctypes/array.rsRepository: RustPython/RustPython
Length of output: 633
🏁 Script executed:
#!/bin/bash
# Search for Constructor trait definition to understand default behavior
rg -n "trait Constructor" --type rsRepository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Look for Constructor trait definition
rg -n "trait Constructor" --type-list | head -20Repository: RustPython/RustPython
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Search for Constructor trait definition more broadly
rg -B2 -A15 "trait Constructor"Repository: RustPython/RustPython
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Check if PyCArrayType has pyclass macro and what it generates
rg -B10 "impl Constructor for PyCArrayType" crates/vm/src/stdlib/ctypes/array.rs | head -40Repository: RustPython/RustPython
Length of output: 390
🏁 Script executed:
#!/bin/bash
# Check PyCUnionType mentioned in the review for comparison
fd ".*union.*\.rs" crates/vm/src/stdlib/ctypes/ --type fRepository: RustPython/RustPython
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check PyCUnionType implementation for comparison
rg -B5 -A25 "impl Constructor for PyCUnionType" crates/vm/src/stdlib/ctypes/union.rsRepository: RustPython/RustPython
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Check if there's a pyclass macro that might generate slot_new for PyCArrayType
rg -B30 "impl Constructor for PyCArrayType" crates/vm/src/stdlib/ctypes/array.rs | grep -A30 "#\[pyclass"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if PyCArrayType is instantiated or constructed anywhere in tests
rg -n "PyCArrayType" crates/vm/src/stdlib/ctypes/ --type rs -A2 -B2 | head -60Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Double-check: look at full PyCArrayType impl block to confirm no slot_new anywhere
sed -n '75,150p' crates/vm/src/stdlib/ctypes/array.rsRepository: RustPython/RustPython
Length of output: 2799
🏁 Script executed:
#!/bin/bash
# Check if there's any pyslot macro or slot_new definition for PyCArrayType outside the Constructor impl
rg -A100 "#\[pyclass.*PyCArrayType" crates/vm/src/stdlib/ctypes/array.rs | grep -i "slot_new\|pyslot"Repository: RustPython/RustPython
Length of output: 101
PyCArrayType lacks a slot_new override and will panic when constructed.
PyCArrayType implements Constructor with only an unreachable py_new, but provides no slot_new override. The Constructor trait's default slot_new implementation calls py_new, which will panic with unreachable!() at runtime.
PyCUnionType demonstrates the correct pattern: override slot_new with custom implementation and keep py_new as unreachable. Add a similar slot_new implementation for PyCArrayType.
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/ctypes/array.rs around lines 75–81, the Constructor impl
for PyCArrayType leaves py_new as unreachable but does not override slot_new, so
the default slot_new will call py_new and panic; add a slot_new override
(modeled on PyCUnionType's slot_new) that accepts the VM and class, extracts and
validates the constructor arguments, initializes and returns a PyCArrayType
instance, and keep py_new as unreachable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/vm/src/stdlib/ctypes/array.rs (1)
78-79: Critical:PyCArrayTypestill lacksslot_newoverride.This issue was previously identified:
PyCArrayTypeimplementsConstructorwith only an unreachablepy_new, but provides noslot_newoverride. The defaultslot_newfrom theConstructortrait will callpy_new, which panics withunreachable!()at runtime.As noted in the previous review, you need to add a
slot_newoverride (similar toPyCUnionType's implementation) that handles the actual construction logic.
🧹 Nitpick comments (8)
crates/stdlib/src/sqlite.rs (1)
652-656: Consider removing unnecessary factory clone.The factory is cloned on line 653 and then immediately passed as a reference to
py_newon line 654. Since you already haveargs.factoryavailable, you could pass&args.factorydirectly to avoid the allocation:fn connect(args: ConnectArgs, vm: &VirtualMachine) -> PyResult { - let factory = args.factory.clone(); - let conn = Connection::py_new(&factory, args, vm)?; - conn.into_ref_with_type(vm, factory).map(Into::into) + let conn = Connection::py_new(&args.factory, args, vm)?; + conn.into_ref_with_type(vm, args.factory).map(Into::into) }crates/vm/src/stdlib/ctypes/function.rs (1)
332-334: Consider adding a doc comment to explain the unreachable stub.While the
unreachable!("use slot_new")pattern matches the reference implementation inpointer.rs, a doc comment would help future maintainers understand the migration pattern and whyslot_newshould be used instead.Add a doc comment:
+ /// This method is intentionally unreachable. Use `slot_new` instead. + /// This stub exists to satisfy the `Constructor` trait with the new signature. fn py_new(_cls: &Py<PyType>, _args: Self::Args, _vm: &VirtualMachine) -> PyResult<Self> { unreachable!("use slot_new") }crates/vm/src/stdlib/typevar.rs (3)
264-270: Consider simplifying trait qualification.The explicit
<Self as Constructor>::py_newqualification on line 265 appears unnecessary unless there's trait ambiguity. Consider simplifying toSelf::py_newfor better readability.- let typevar = <Self as Constructor>::py_new(&cls, args, vm)?; + let typevar = Self::py_new(&cls, args, vm)?;
539-614: Inconsistent constructor patterns across similar types.
TypeVardelegatesslot_new→py_new(with a functionalpy_new), whileParamSpecandTypeVarTupleimplement all logic inslot_newwith unreachablepy_newstubs. Consider standardizing to one pattern for better maintainability and code clarity.Options:
- Make all three delegate to
py_new(likeTypeVar)- Make all three inline with unreachable stubs (like
ParamSpec/TypeVarTuple)- Add clarifying comments explaining why different patterns are used
Also applies to: 725-776
613-613: Consider more descriptive unreachable messages.The
unreachable!messages could be more informative to help developers understand why this code path should never be reached.- unreachable!("use slot_new") + unreachable!("ParamSpec::py_new should never be called directly; construction must go through slot_new to properly set __module__")Apply similar change to
TypeVarTuple::py_newat line 775.Also applies to: 775-775
crates/vm/src/stdlib/functools.rs (1)
207-211: Consider using the shorter type path for consistency.Other files in this PR use
&Py<PyType>directly (with appropriate imports), while this file uses the fully qualified&crate::Py<crate::builtins::PyType>. Consider importingPyTypefrombuiltinsto maintain consistency:use crate::{ - Py, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, - builtins::{PyDict, PyGenericAlias, PyTuple, PyTypeRef}, + Py, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, + builtins::{PyDict, PyGenericAlias, PyTuple, PyType, PyTypeRef}, ... }; - fn py_new( - _cls: &crate::Py<crate::builtins::PyType>, + fn py_new( + _cls: &Py<PyType>, args: Self::Args, vm: &VirtualMachine, ) -> PyResult<Self> {crates/vm/src/stdlib/ctypes/base.rs (1)
779-806: Call sites usingFuncArgsto reach PyCSimple::slot_new are correct; consider de-duplication
from_address,from_buffer,from_buffer_copy, andin_dllnow all:
- Decode raw bytes into a Python
valuewithbytes_to_pyobject, then- Call
PyCSimple::slot_new(cls.clone(), FuncArgs::new(vec![value], KwArgs::default()), vm),which is equivalent to invoking
cls(value)once, preserving the previous API.If you’d like to reduce repetition, you could add a small helper like
fn new_with_value(cls: PyTypeRef, value: PyObjectRef, vm: &VirtualMachine) -> PyResultthat wraps theFuncArgscreation andslot_newcall.Also applies to: 815-856, 868-903, 905-971
crates/vm/src/stdlib/typing.rs (1)
76-91: NoDefault constructor: singleton behavior is correct; py_new could be tightened
slot_newcorrectly:
- Rejects any positional/keyword arguments, and
- Always returns the
vm.ctx.typing_no_defaultsingleton.Given that,
py_newreturning a bareSelfis only meaningful for internal Rust callers; to avoid accidental creation of extra sentinels later, you might consider makingpy_newunreachable!("use slot_new")as done for other types, unless you explicitly rely on it for context setup.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
crates/stdlib/src/array.rs(3 hunks)crates/stdlib/src/bz2.rs(4 hunks)crates/stdlib/src/contextvars.rs(4 hunks)crates/stdlib/src/csv.rs(1 hunks)crates/stdlib/src/json.rs(3 hunks)crates/stdlib/src/lzma.rs(5 hunks)crates/stdlib/src/mmap.rs(6 hunks)crates/stdlib/src/overlapped.rs(3 hunks)crates/stdlib/src/pystruct.rs(2 hunks)crates/stdlib/src/select.rs(2 hunks)crates/stdlib/src/sqlite.rs(6 hunks)crates/stdlib/src/ssl.rs(6 hunks)crates/stdlib/src/zlib.rs(3 hunks)crates/vm/src/builtins/bool.rs(3 hunks)crates/vm/src/builtins/bytes.rs(2 hunks)crates/vm/src/builtins/classmethod.rs(3 hunks)crates/vm/src/builtins/code.rs(3 hunks)crates/vm/src/builtins/complex.rs(3 hunks)crates/vm/src/builtins/enumerate.rs(1 hunks)crates/vm/src/builtins/filter.rs(1 hunks)crates/vm/src/builtins/float.rs(3 hunks)crates/vm/src/builtins/function.rs(5 hunks)crates/vm/src/builtins/genericalias.rs(2 hunks)crates/vm/src/builtins/int.rs(3 hunks)crates/vm/src/builtins/list.rs(1 hunks)crates/vm/src/builtins/map.rs(1 hunks)crates/vm/src/builtins/mappingproxy.rs(1 hunks)crates/vm/src/builtins/memory.rs(1 hunks)crates/vm/src/builtins/object.rs(2 hunks)crates/vm/src/builtins/property.rs(3 hunks)crates/vm/src/builtins/set.rs(3 hunks)crates/vm/src/builtins/singletons.rs(3 hunks)crates/vm/src/builtins/slice.rs(1 hunks)crates/vm/src/builtins/staticmethod.rs(2 hunks)crates/vm/src/builtins/str.rs(2 hunks)crates/vm/src/builtins/super.rs(1 hunks)crates/vm/src/builtins/traceback.rs(2 hunks)crates/vm/src/builtins/tuple.rs(3 hunks)crates/vm/src/builtins/type.rs(2 hunks)crates/vm/src/builtins/weakproxy.rs(2 hunks)crates/vm/src/builtins/weakref.rs(2 hunks)crates/vm/src/builtins/zip.rs(2 hunks)crates/vm/src/exceptions.rs(1 hunks)crates/vm/src/protocol/object.rs(3 hunks)crates/vm/src/stdlib/ast/python.rs(2 hunks)crates/vm/src/stdlib/collections.rs(3 hunks)crates/vm/src/stdlib/ctypes/array.rs(4 hunks)crates/vm/src/stdlib/ctypes/base.rs(7 hunks)crates/vm/src/stdlib/ctypes/function.rs(5 hunks)crates/vm/src/stdlib/ctypes/pointer.rs(3 hunks)crates/vm/src/stdlib/ctypes/structure.rs(4 hunks)crates/vm/src/stdlib/ctypes/union.rs(4 hunks)crates/vm/src/stdlib/functools.rs(2 hunks)crates/vm/src/stdlib/io.rs(2 hunks)crates/vm/src/stdlib/itertools.rs(26 hunks)crates/vm/src/stdlib/operator.rs(5 hunks)crates/vm/src/stdlib/posix.rs(2 hunks)crates/vm/src/stdlib/thread.rs(2 hunks)crates/vm/src/stdlib/typevar.rs(10 hunks)crates/vm/src/stdlib/typing.rs(4 hunks)crates/vm/src/types/slot.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/vm/src/protocol/object.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/stdlib/posix.rs
- crates/vm/src/builtins/traceback.rs
- crates/vm/src/builtins/int.rs
- crates/vm/src/builtins/staticmethod.rs
- crates/vm/src/builtins/memory.rs
- crates/vm/src/builtins/float.rs
- crates/vm/src/builtins/weakproxy.rs
- crates/vm/src/builtins/object.rs
- crates/vm/src/builtins/singletons.rs
- crates/stdlib/src/lzma.rs
- crates/vm/src/stdlib/ast/python.rs
- crates/vm/src/builtins/classmethod.rs
- crates/vm/src/stdlib/ctypes/union.rs
- crates/stdlib/src/array.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/complex.rscrates/vm/src/builtins/slice.rscrates/vm/src/stdlib/io.rscrates/vm/src/builtins/filter.rscrates/stdlib/src/overlapped.rscrates/vm/src/builtins/property.rscrates/vm/src/stdlib/thread.rscrates/vm/src/builtins/enumerate.rscrates/vm/src/builtins/genericalias.rscrates/vm/src/builtins/weakref.rscrates/vm/src/builtins/zip.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/builtins/bytes.rscrates/vm/src/stdlib/typing.rscrates/vm/src/builtins/map.rscrates/stdlib/src/pystruct.rscrates/vm/src/builtins/function.rscrates/vm/src/stdlib/ctypes/function.rscrates/vm/src/exceptions.rscrates/stdlib/src/mmap.rscrates/vm/src/builtins/set.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/builtins/tuple.rscrates/stdlib/src/json.rscrates/stdlib/src/csv.rscrates/vm/src/stdlib/ctypes/array.rscrates/vm/src/stdlib/collections.rscrates/vm/src/builtins/type.rscrates/vm/src/builtins/code.rscrates/vm/src/stdlib/functools.rscrates/vm/src/builtins/bool.rscrates/vm/src/builtins/super.rscrates/vm/src/stdlib/ctypes/structure.rscrates/vm/src/builtins/list.rscrates/vm/src/types/slot.rscrates/vm/src/stdlib/operator.rscrates/vm/src/builtins/mappingproxy.rscrates/stdlib/src/sqlite.rscrates/stdlib/src/ssl.rscrates/stdlib/src/zlib.rscrates/stdlib/src/select.rscrates/stdlib/src/contextvars.rscrates/stdlib/src/bz2.rscrates/vm/src/stdlib/typevar.rscrates/vm/src/stdlib/itertools.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/stdlib/src/overlapped.rscrates/vm/src/exceptions.rscrates/vm/src/stdlib/itertools.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/weakref.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/stdlib/typing.rscrates/stdlib/src/pystruct.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/tuple.rscrates/stdlib/src/json.rscrates/stdlib/src/ssl.rscrates/vm/src/stdlib/typevar.rs
🧬 Code graph analysis (33)
crates/vm/src/builtins/complex.rs (4)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/int.rs (1)
slot_new(210-246)crates/vm/src/builtins/singletons.rs (2)
slot_new(42-45)slot_new(96-99)crates/vm/src/builtins/str.rs (1)
slot_new(353-379)
crates/vm/src/builtins/slice.rs (1)
crates/vm/src/builtins/singletons.rs (2)
slot_new(42-45)slot_new(96-99)
crates/stdlib/src/overlapped.rs (3)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/vm/src/stdlib/posix.rs (1)
py_new(835-839)
crates/vm/src/builtins/property.rs (3)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/builtins/enumerate.rs (1)
crates/vm/src/builtins/function.rs (3)
new(58-91)new(783-785)new(905-909)
crates/vm/src/builtins/genericalias.rs (1)
crates/stdlib/src/csv.rs (1)
py_new(71-73)
crates/vm/src/builtins/weakref.rs (3)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/int.rs (1)
slot_new(210-246)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/builtins/zip.rs (2)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/vm/src/stdlib/posix.rs (1)
args(1640-1647)
crates/vm/src/stdlib/ctypes/pointer.rs (2)
crates/vm/src/stdlib/ctypes/array.rs (3)
slot_new(252-305)py_new(78-80)py_new(307-309)crates/vm/src/stdlib/ctypes/base.rs (2)
slot_new(613-662)py_new(664-666)
crates/vm/src/builtins/bytes.rs (3)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/stdlib/typing.rs (1)
crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/map.rs (6)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/lzma.rs (2)
py_new(151-171)py_new(367-394)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/stdlib/src/select.rs (1)
py_new(579-587)
crates/stdlib/src/pystruct.rs (2)
crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/sqlite.rs (2)
py_new(856-883)py_new(1943-1949)
crates/vm/src/stdlib/ctypes/function.rs (1)
crates/vm/src/stdlib/ctypes/pointer.rs (2)
slot_new(79-90)py_new(92-94)
crates/vm/src/exceptions.rs (5)
crates/vm/src/builtins/bool.rs (2)
slot_new(104-115)class(90-92)crates/vm/src/builtins/bytes.rs (3)
slot_new(97-100)class(84-86)class(698-700)crates/vm/src/builtins/int.rs (1)
slot_new(210-246)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/slice.rs (2)
slot_new(67-91)slot_new(316-319)
crates/stdlib/src/mmap.rs (3)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/lzma.rs (2)
py_new(151-171)py_new(367-394)
crates/vm/src/builtins/set.rs (3)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/stdlib/ctypes/base.rs (1)
crates/vm/src/stdlib/ctypes/function.rs (4)
slot_new(189-330)args(302-302)args(366-376)value(52-52)
crates/vm/src/builtins/tuple.rs (4)
crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/stdlib/src/json.rs (3)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/vm/src/builtins/type.rs (1)
crates/vm/src/builtins/object.rs (2)
slot_new(35-111)py_new(113-115)
crates/vm/src/builtins/code.rs (4)
crates/vm/src/stdlib/ctypes/function.rs (2)
args(302-302)args(366-376)crates/vm/src/stdlib/typevar.rs (2)
args(192-192)args(427-433)crates/vm/src/builtins/staticmethod.rs (1)
new(69-73)crates/vm/src/builtins/str.rs (1)
new(2076-2078)
crates/vm/src/stdlib/functools.rs (2)
crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/vm/src/builtins/bool.rs (2)
crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/tuple.rs (1)
zelf(276-280)
crates/vm/src/stdlib/ctypes/structure.rs (3)
crates/vm/src/stdlib/ctypes/array.rs (3)
slot_new(252-305)py_new(78-80)py_new(307-309)crates/vm/src/stdlib/ctypes/base.rs (2)
slot_new(613-662)py_new(664-666)crates/vm/src/stdlib/ctypes/pointer.rs (2)
slot_new(79-90)py_new(92-94)
crates/vm/src/types/slot.rs (1)
crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/stdlib/src/sqlite.rs (2)
crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/stdlib/src/ssl.rs (2)
crates/stdlib/src/sqlite.rs (4)
py_new(856-883)new(396-401)new(1545-1563)new(2599-2646)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/stdlib/src/zlib.rs (8)
crates/stdlib/src/array.rs (1)
py_new(654-706)crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/mmap.rs (3)
py_new(391-490)py_new(493-638)args(913-933)crates/stdlib/src/overlapped.rs (1)
py_new(267-299)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/vm/src/stdlib/io.rs (1)
vm(3478-3480)
crates/stdlib/src/contextvars.rs (4)
crates/stdlib/src/pystruct.rs (1)
py_new(244-248)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/stdlib/src/bz2.rs (3)
crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/pystruct.rs (1)
py_new(244-248)
crates/vm/src/stdlib/typevar.rs (3)
crates/vm/src/builtins/float.rs (1)
slot_new(134-153)crates/vm/src/builtins/int.rs (1)
slot_new(210-246)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/stdlib/itertools.rs (1)
crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
⏰ 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 rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- 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)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (97)
crates/stdlib/src/ssl.rs (5)
40-42: LGTM!The import additions for
PyTypeandFuncArgsare correctly added to support the refactored constructor signatures throughout this file.
2291-2296: LGTM!The signature refactor correctly changes the parameter from
PyTypeRefto&Py<PyType>and the return type fromPyResulttoPyResult<Self>, aligning with the PR's objective.
2360-2395: LGTM!Returning
Ok(PySSLContext { ... })directly instead of going throughinto_ref_with_type(vm)is the correct pattern for this refactor. The struct initialization logic remains unchanged.
4112-4120: Acceptable pattern for non-instantiable types.The
unreachable!("use slot_new")pattern is intentional here sincePySSLSocketcannot be directly instantiated—users must useSSLContext.wrap_socket(). Theslot_newhandles the actual entry point and returns a properTypeError.One minor consideration: if a future framework change inadvertently routes through
py_new, this will panic rather than return an error. However, since this is by design to catch framework bugs and matches the PR's pattern for slot-based constructors, this is acceptable.
4213-4217: LGTM!Clean refactor following the standard pattern: signature updated to
&Py<PyType>, return type toPyResult<Self>, and direct struct construction withoutinto_ref_with_type.crates/stdlib/src/sqlite.rs (3)
856-883: LGTM!The constructor signature refactoring looks correct. The change from
PyTypeRefto&Py<PyType>and returningPyResult<Self>instead of usinginto_ref_with_typeinternally follows the new pattern consistently. The base class detection usingcls.is()is idiomatic.
1943-1949: LGTM!The refactored
py_newsignature is correct and consistent with the PR's objective. The unused_clsparameter is properly prefixed with an underscore to suppress warnings.
2114-2126: LGTM!The constructor refactoring is implemented correctly. The signature matches the new pattern, and the implementation properly returns
PyResult<Self>with appropriate error handling.crates/vm/src/stdlib/ctypes/function.rs (5)
189-189: Constructor refactoring follows the correct pattern.The rename from
py_newtoslot_newand the return type ofPyResult(i.e.,PyResult<PyObjectRef>) align with the reference implementation inpointer.rs.
196-206: Empty args constructor branch correctly implements the new pattern.The explicit field initialization and the conversion via
.into_ref_with_type(vm, cls).map(Into::into)are properly applied.
214-224: Integer address constructor branch correctly implements the new pattern.The pointer extraction, field initialization, and conversion are properly applied. The logic remains functionally equivalent to the previous implementation.
273-283: Tuple form constructor branch correctly implements the new pattern.The library function loading and subsequent object construction with
.into_ref_with_type(vm, cls).map(Into::into)are properly applied.
316-326: Callable thunk constructor branch correctly implements the new pattern.The thunk creation, lifecycle management (via
needs_freeand storingthunk_refinhandler), and conversion are all properly applied.crates/stdlib/src/zlib.rs (3)
13-14: LGTM: Import changes are minimal and necessary.The addition of
PyandPyTypesupports the new constructor signature pattern, whilePyTypeRefis retained for theerrorfunction at line 57. All imports are properly used.
573-573: LGTM: Constructor signature updated consistently with the refactoring pattern.The signature change from
cls: PyTypeRefto_cls: &Py<PyType>and return typePyResult<Self>aligns with the codebase-wide refactor. The underscore prefix correctly indicates the parameter is intentionally unused.
583-585: LGTM: Constructor body correctly simplified.The direct return of
Ok(Self { inner: PyMutex::new(inner) })follows the established pattern across the codebase. The refactor simplifies the code while preserving all initialization and error-handling logic.crates/vm/src/stdlib/operator.rs (4)
7-7: LGTM!The addition of
PyTypeto the imports is necessary for the refactored constructor signatures.
467-476: LGTM! Constructor refactored consistently with PyAttrGetter.The implementation correctly preserves validation logic and follows the same refactoring pattern as the other constructors in this file.
551-561: LGTM! PyMethodCaller constructor refactored consistently.The refactoring maintains the original validation logic while adopting the new direct-return pattern. The parameter destructuring is appropriate for this constructor's tuple-based Args type.
391-409: LGTM! Constructor refactored to return Self directly.The refactoring correctly preserves all validation logic while simplifying the return path. The
_clsparameter is appropriately marked as unused since the class reference is no longer needed for the conversion step.crates/vm/src/stdlib/itertools.rs (19)
10-10: LGTM!Import correctly adds
PyTypealongside existingPyTypeRef- both are needed sincePyTypeis used in the newpy_newsignatures whilePyTypeRefis still used inslot_newmethods and__reduce__return types.
200-210: LGTM!Clean refactor following the new constructor pattern. Unused parameters correctly prefixed with underscore.
257-276: LGTM!Correctly retains
vmparameter (used for default values and type checking) while updating the constructor pattern.
323-333: LGTM!Simple and correct constructor refactor.
379-397: LGTM!Constructor correctly updated while preserving the times conversion logic.
465-475: LGTM!Starmap constructor correctly refactored.
525-542: LGTM!Takewhile constructor correctly refactored with proper initialization of
stop_flag.
611-628: LGTM!Dropwhile constructor correctly refactored.
733-747: LGTM!GroupBy constructor correctly refactored.
1022-1038: LGTM!FilterFalse constructor correctly refactored.
1096-1107: LGTM!Accumulate constructor cleanly refactored.
1311-1337: LGTM!Product constructor correctly refactored, preserving the pools collection and initialization logic.
1467-1493: LGTM!Combinations constructor correctly refactored with preserved validation logic.
1598-1622: LGTM!CombinationsWithReplacement constructor correctly refactored.
1694-1731: LGTM!Permutations constructor correctly refactored, preserving the complex optional
rparameter handling.
1824-1839: LGTM!ZipLongest constructor correctly refactored.
1912-1921: LGTM!Pairwise constructor correctly refactored.
1973-2001: LGTM!Batched constructor correctly refactored with preserved validation logic for the
nparameter.
1240-1261: Theunreachable!guard is safe and follows established patterns in the codebase.The
Constructortrait's defaultslot_newimplementation (lines 812–816 incrates/vm/src/types/slot.rs) callspy_newonly whenslot_newis not overridden. SincePyItertoolsTeeoverridesslot_new, the default implementation is bypassed entirely, andpy_newis never invoked. Theunreachable!("use slot_new")guard prevents runtime panics only in cases of programmer error (accidental direct calls).This pattern is idiomatic in RustPython: 25+ classes across builtins, stdlib, and exceptions modules use the same approach for special construction requirements (e.g.,
PySSLSocket,PyBaseException,bool,tuple,str). No changes required.crates/vm/src/stdlib/typevar.rs (3)
3-4: LGTM: Import updates support constructor signature changes.The addition of
PyandPyTypeimports aligns with the PR's objective to use&Py<PyType>parameters in constructors.
819-822: LGTM: Constructor signatures updated correctly.The signature changes from
PyTypeRefto&Py<PyType>and direct return ofSelfalign with the PR objectives. The simpler pattern for these types (no customslot_new, no module setting) is appropriate given their nature as lightweight wrappers.Also applies to: 897-900
1-952: Run cargo fmt and cargo clippy before finalizing this PR.The sandbox environment cannot execute Cargo tools directly, so you must verify locally that the code passes formatting and linting checks:
cargo fmtto ensure code formatting compliancecargo clippy -- -D warningsto catch any lintsThis is required per the coding guidelines before merging.
crates/vm/src/exceptions.rs (2)
710-717: LGTM! Transition to slot-based constructor pattern.The rename from
py_newtoslot_newand the implementation look correct. This follows the PR-wide refactoring strategy whereslot_newserves as the actual constructor implementation while maintaining the old signature pattern (usinginto_ref_with_type).
719-721: [Your rewritten review comment text here]
[Exactly ONE classification tag]crates/vm/src/builtins/genericalias.rs (1)
61-72: LGTM!The constructor signature change from
cls: PyTypeRefto_cls: &Py<PyType>and return type toPyResult<Self>is consistent with the PR's refactoring pattern. The directOk(Self::new(...))return eliminates the intermediateinto_ref_with_typeconversion.crates/vm/src/builtins/filter.rs (1)
27-36: LGTM!The constructor refactoring is correct. The unused parameters
_clsand_vmare properly prefixed, and the directSelfconstruction simplifies the code while maintaining the same semantics.crates/vm/src/builtins/zip.rs (2)
1-1: LGTM!The import cleanup removing
PyTypeRefis correct since the type is no longer explicitly referenced in the constructor signature.
36-44: LGTM!The constructor refactoring follows the established pattern. The
PyAtomic<bool>initialization withRadium::new()is preserved correctly, and the directSelfreturn simplifies the construction path.crates/vm/src/stdlib/thread.rs (2)
9-9: LGTM!Adding
PyTypeto the imports is necessary for the updatedpy_newsignature.
174-177: LGTM!The signature change is correct. Even though this constructor always returns an error (Lock instances are created via
allocate_lock()), thePyResult<Self>return type is valid sinceErr(...)is compatible with anyResult<T, E>.crates/vm/src/builtins/map.rs (1)
29-36: LGTM!The constructor refactoring is consistent with the PR pattern. The direct
Selfconstruction eliminates theinto_ref_with_typeindirection while preserving the same behavior.crates/stdlib/src/csv.rs (1)
71-73: LGTM! Constructor refactoring is correct.The simplification from
into_ref_with_type(vm, cls).map(Into::into)to directSelf::try_from_object(vm, ctx)correctly aligns with the PR's goal of returningSelfdirectly. The_clsparameter is appropriately unused with the underscore prefix.crates/vm/src/builtins/slice.rs (1)
316-323: LGTM! Singleton pattern correctly implemented.The changes properly implement the slot-based constructor for the Ellipsis singleton:
slot_newcorrectly binds arguments and returns the singleton instancepy_newappropriately usesunreachable!with a clear message since Ellipsis is a singleton- This pattern matches other singletons like
PyNoneandPyNotImplemented(see relevant snippets)crates/stdlib/src/select.rs (1)
579-587: LGTM! PyEpoll constructor correctly refactored.The constructor signature and return path have been properly updated:
- Parameter validation logic preserved
- Direct
Self::new().map_err(...)correctly returnsPyResult<Self>- Error handling remains intact
crates/vm/src/builtins/mappingproxy.rs (1)
64-79: LGTM! MappingProxy constructor correctly simplified.The constructor has been properly refactored:
- Validation logic for mapping types preserved
- Direct
Ok(Self { mapping: ... })correctly returns the instance- Error handling remains intact with appropriate type error for invalid mappings
crates/vm/src/builtins/property.rs (2)
339-347: Constructor refactoring looks correct.The
py_newsignature has been properly updated to returnPyResult<Self>with direct construction. All fields are properly initialized.
227-227: No issues found.PyPropertyhas aslot_newmethod provided by theConstructortrait (which is included in the#[pyclass]macro withwith(Constructor, ...)). TheConstructortrait definesslot_newas a default method that wraps thepy_newimplementation, so the call on line 227 is valid and will compile correctly.crates/vm/src/builtins/enumerate.rs (1)
43-53: LGTM! PyEnumerate constructor properly refactored.The constructor has been correctly simplified:
- Counter initialization logic preserved
- Direct
Ok(Self { counter: ..., iterable })correctly returns the instance- Unused parameters appropriately prefixed with underscores
crates/vm/src/builtins/code.rs (1)
393-511: LGTM! PyCode constructor correctly refactored.The constructor has been properly updated with all the complex validation and construction logic preserved:
- All parameter validation remains intact
- Constants, names, varnames, freevars, cellvars conversion logic preserved
- Direct
Ok(PyCode::new(code))correctly returns the instance- Error handling throughout the constructor remains appropriate
crates/stdlib/src/overlapped.rs (1)
267-299: LGTM! Overlapped constructor correctly refactored.The Windows-specific constructor has been properly updated:
- Event creation and validation logic preserved
- OVERLAPPED structure initialization remains correct
- Direct
Ok(Overlapped { inner: PyMutex::new(inner) })correctly returns the instance- Error handling for Windows API calls intact
crates/vm/src/builtins/super.rs (1)
50-58: LGTM!The constructor signature change from
PyTypeRefto&Py<PyType>and returningPyResult<Self>directly aligns with the PR's objective to standardize constructor patterns. The framework will handle wrappingSelfwith the appropriate type.crates/vm/src/builtins/list.rs (1)
379-385: LGTM!The constructor correctly returns an empty
Self::default(), delegating element initialization to theInitializer::initmethod. This follows Python's two-phase initialization pattern (__new__+__init__).crates/vm/src/builtins/bool.rs (1)
104-119: LGTM!The
slot_newpattern is appropriate forPyBoolsince Python'sbooltype returns singletonTrue/Falseobjects rather than creating new instances. The unreachablepy_newshim correctly guides callers to useslot_newinstead.crates/stdlib/src/mmap.rs (2)
391-490: LGTM!The Unix
py_newimplementation correctly transitions to the new signature pattern, returningOk(Self { ... })directly. The complex mmap initialization logic and error handling are preserved.
492-638: LGTM!The Windows
py_newimplementation mirrors the Unix changes with the same signature pattern. Both platform-specific implementations now consistently returnPyResult<Self>.crates/vm/src/stdlib/functools.rs (1)
237-244: LGTM!The
PyPartialconstructor correctly returnsOk(Self { ... })directly. The complex logic for handling nested partials and merging keyword arguments is preserved.crates/stdlib/src/bz2.rs (3)
10-17: LGTM! Import changes align with the new constructor API.The imports are correctly updated to use
Py<PyType>pattern and movePyResultto the object module, consistent with the PR's refactoring approach.
64-68: LGTM! BZ2Decompressor constructor correctly refactored.The signature change to
_cls: &Py<PyType>withPyResult<Self>return type follows the established pattern. The_clsprefix correctly indicates the parameter is unused, and directOk(Self { ... })construction is appropriate since this type doesn't support subclassing with custom class references.
133-154: LGTM! BZ2Compressor constructor correctly refactored.The refactored signature and argument destructuring
(compresslevel,): Self::Argsis clean. The validation logic and directOk(Self { ... })return are correct. This pattern matches the other stdlib constructors likecsv.rsandpystruct.rsfrom the relevant snippets.crates/vm/src/builtins/function.rs (4)
4-7: LGTM! Import updated to reflect new type usage.Changing from
PyTypeReftoPyTypealigns with the constructor signature changes where&Py<PyType>is now used.
673-714: LGTM! PyFunction constructor properly refactored with preserved logic.The complex constructor logic is correctly preserved:
- Closure validation with length check against
code.freevars- Typed closure conversion via
try_into_typed::<PyCell>- Name/qualname setting when provided
- Defaults and kwdefaults application
The
_clsparameter is appropriately unused sincePyFunctionconstruction doesn't vary by subclass type.
773-779: LGTM! PyBoundMethod constructor correctly simplified.The direct
Ok(Self::new(object, function))return is clean and appropriate for this simple constructor.
898-900: LGTM! PyCell constructor correctly refactored.Simple and correct - the
_vmparameter is now unused (prefixed with_), and the directOk(Self::new(...))return follows the established pattern.crates/vm/src/builtins/weakref.rs (2)
9-9: LGTM! FuncArgs import added for slot-based constructor.Required for the
args.bind(vm)?pattern inslot_new.
41-49: LGTM! Correctly uses slot_new pattern since cls is required.The
slot_newapproach is appropriate here becausePyWeakneeds the actualcls: PyTypeRefto pass todowngrade_with_typ. Theargs.bind(vm)?extracts theWeakNewArgs, and the unreachablepy_newplaceholder correctly signals thatslot_newis the intended entry point.This pattern matches other builtins like
bytes.rs,staticmethod.rs, andint.rsthat also require the class reference.crates/vm/src/builtins/complex.rs (2)
9-9: LGTM! FuncArgs import added for slot-based binding.
154-232: LGTM! Complex constructor correctly uses slot_new pattern.The
slot_newapproach is necessary becausePyComplexneedscls: PyTypeReffor:
- Exact-type checking:
cls.is(vm.ctx.types.complex_type)for early return optimization- Subclass support:
into_ref_with_type(vm, cls)when constructing new instancesThe complex parsing logic (string handling, real/imaginary component calculation) is correctly preserved. The unreachable
py_newplaceholder follows the established pattern.crates/vm/src/builtins/set.rs (2)
15-15: LGTM! FuncArgs import added for slot-based binding.
923-950: LGTM! PyFrozenSet constructor correctly uses slot_new pattern.The
slot_newapproach is required becausePyFrozenSetneedscls: PyTypeReffor:
- Exact-type early return:
downcast_exact::<Self>(vm)optimization- Empty frozenset singleton:
cls.is(vm.ctx.types.frozenset_type)check- Subclass construction:
into_ref_with_type(vm, cls)The construction logic is correctly preserved, including the empty set singleton optimization and proper element iteration. The unreachable
py_newplaceholder follows the established pattern seen across the codebase.crates/stdlib/src/contextvars.rs (3)
247-249: LGTM! Constructor correctly returns Self.The updated
py_newsignature using&Py<PyType>and returningPyResult<Self>directly is correct and aligns with the PR's refactoring pattern.
587-589: LGTM! Unreachable stub correctly guides to slot_new.The unreachable
py_newimplementation correctly directs users to useslot_newinstead, which is defined at line 584.
492-512: Slot_new pattern is correct and safe.The Constructor trait routes calls through
slot_new, which is registered as the Python__new__slot via#[pyslot]. Whenslot_newis overridden to perform custom initialization (like hash computation), markingpy_newasunreachable!is the correct pattern—it signals that the instance must exist beforepy_newlogic can run. This approach is consistent throughout the codebase and properly prevents accidental directpy_newcalls that would bypass critical initialization.crates/stdlib/src/json.rs (1)
36-64: LGTM! Constructor correctly updated to return Self.The refactored
py_newmethod properly:
- Updates parameter from
PyTypeRefto&Py<PyType>- Returns
PyResult<Self>directly withOk(Self { ... })- Preserves all field initialization logic
crates/stdlib/src/pystruct.rs (1)
244-248: LGTM! Constructor correctly simplified to return Self.The refactored constructor properly returns
Ok(Self { spec, format })directly instead of the previousinto_ref_with_typechain, matching the PR's pattern.crates/vm/src/stdlib/ctypes/pointer.rs (1)
79-94: LGTM! Slot-based constructor pattern correctly implemented.The changes properly:
- Introduce
slot_newthat bindsFuncArgstoSelf::Args- Extract
initial_contentsfrom the bound args (matching previous behavior)- Use
into_ref_with_typeto create the PyRef (required for this type)- Provide an unreachable
py_newstub to guide callersThe import additions (FuncArgs, Py) support the new constructor pattern.
crates/vm/src/builtins/type.rs (1)
994-1293: LGTM! Type metaclass constructor correctly refactored.The extensive
PyTypeconstructor has been properly migrated:
- Renamed from
py_newtoslot_new(line 994)- Added unreachable
py_newstub (lines 1291-1293)- Preserved all complex type creation logic (bases resolution, MRO calculation, slots setup, etc.)
The
Argstype is alreadyFuncArgs, so no binding step is needed at the top ofslot_new.crates/vm/src/builtins/bytes.rs (1)
97-104: LGTM! Bytes constructor correctly migrated to slot-based pattern.The changes properly:
- Add
slot_newthat bindsFuncArgstoSelf::Args(ByteInnerNewOptions)- Call
options.get_bytes(cls, vm)which handles the actual bytes creation- Provide unreachable
py_newstubThe
FuncArgsimport addition supports this pattern.crates/vm/src/stdlib/ctypes/structure.rs (2)
28-48: LGTM! Metaclass constructor correctly delegates to PyType::slot_new.The refactored constructor:
- Updates to
slot_newsignature withFuncArgs- Correctly calls
PyType::slot_new(line 30) instead of the oldpy_new- Preserves field processing logic
- Provides unreachable
py_newstubThis aligns with the refactored
PyTypeconstructor intype.rs.
246-336: LGTM! Structure constructor correctly implements slot-based pattern.The comprehensive constructor migration:
- Introduces
slot_newwithFuncArgsparameter- Processes
_fields_from the class attributes- Builds field map and initializes buffer
- Creates instance via
into_ref_with_type- Applies field values from positional/keyword arguments
- Provides unreachable
py_newstubAll initialization logic is preserved, and the pattern matches the broader PR refactoring.
crates/vm/src/stdlib/ctypes/array.rs (1)
252-252: LGTM! Constructor refactor follows the correct pattern.The refactor correctly implements the new constructor pattern:
slot_new(line 252) serves as the primary constructor entry point with full implementation logicpy_new(lines 307-309) is markedunreachable!("use slot_new")to guide callers towardslot_newThis aligns with the
Constructortrait design where types needing special handling (beyond simple payload creation) should overrideslot_newdirectly.Also applies to: 307-309
crates/vm/src/types/slot.rs (1)
782-821: LGTM! Well-documented trait refactor.The
Constructortrait refactor is well-designed:
- Clear documentation (lines 782-803) explains when to use
py_new(most cases) vs. overridingslot_new(special handling)- The signature change to
&Py<PyType>(line 820) aligns with the PR's uniform refactor objectives- Default
slot_newimplementation (lines 812-816) correctly delegates topy_newand wraps the resultThis provides a clean separation of concerns:
py_newfor simple payload creation,slot_newfor complex construction logic.crates/vm/src/stdlib/io.rs (2)
3638-3652: StringIO::py_new migration toPy<PyType>/PyResult<Self>looks correctThe updated
py_newcorrectly ignores_cls/_vmand returns an ownedStringIObuilt fromobject, matching prior semantics while conforming to the newConstructorAPI.
3768-3781: BytesIO::py_new updated in line with new constructor convention
BytesIO::py_newnow takes_cls: &Py<PyType>and returnsPyResult<Self>, constructing the internal buffer and flags exactly as before; this is consistent with the new pattern and doesn’t alter behavior.crates/vm/src/builtins/tuple.rs (1)
112-145: PyTuple::slot_new refactor preserves semantics and matches slot-based APIBinding through
FuncArgsintoOptionalArg<PyObjectRef>keeps thetuple([iterable])signature, and the body still:
- Reuses the incoming tuple when appropriate,
- Reuses
ctx.empty_tuplefor empty exact tuples,- Falls back to
into_ref_with_typefor other cases.The
py_newstub beingunreachable!("use slot_new")is consistent with the new pattern whereslot_newis the single entry point.crates/vm/src/stdlib/ctypes/base.rs (1)
6-7: PyCSimple::slot_new and constructor wiring look consistent with ctypes expectationsThe new
slot_newcorrectly:
- Binds
FuncArgsinto a singleOptionalArg,- Enforces presence of
_type_oncls,- Normalizes the incoming value via
set_primitive,- Handles default values per type code,
- Applies endianness via
_swappedbytes_,- Builds
CDataObject::from_bytesand returns an instance viainto_ref_with_type.The unreachable
py_newis appropriate now thatslot_newis the only construction path used by the VM.Also applies to: 610-667
crates/vm/src/stdlib/collections.rs (1)
10-11: Deque iteratorpy_newimplementations align with new constructor patternBoth
_deque_iteratorand_deque_reverse_iteratornow:
- Take
_cls: &Py<PyType>and returnPyResult<Self>,- Correctly initialize via
Self::new(deque)/PyDeque::__reversed__(deque)?,- Optionally adjust
internal.positionfrom theindexargument,- Ignore
_cls/_vmappropriately.This matches the updated
Constructorexpectations without changing the Python-visible behavior (including pickling via__reduce__).Also applies to: 605-620, 677-691
crates/vm/src/stdlib/typing.rs (1)
137-176: TypeAliasType::py_new now matches the newConstructorsignature while preserving checksThe updated
py_new:
- Uses
_cls: &Py<PyType>andFuncArgsasSelf::Args,- Enforces exactly two positional args (
name,value),- Validates
nameasstrand optionaltype_paramsastuple,- Returns
Ok(Self::new(name, type_params, value)).This keeps the Python-facing API and validation intact while conforming to the refactored constructor pattern.
1dc4701 to
10e4ade
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/vm/src/stdlib/ctypes/array.rs (1)
78-80: The prior review concern aboutPyCArrayTypelackingslot_newremains unresolved.
PyCArrayTypestill only provides anunreachable!("use slot_new")py_newwithout overridingslot_new. The defaultConstructor::slot_newwill call thispy_newand panic at runtime.While
PyCArrayTypehas aCallableimpl (lines 41-73), theConstructorpath is broken. Per the relevant snippets, other ctypes (pointer.rs, base.rs) follow the same pattern, so this may be intentional if construction only happens viaCallable::call. However, this should be verified or documented.
🧹 Nitpick comments (13)
crates/stdlib/src/pystruct.rs (1)
241-248:PyStruct::py_newnow matches the newConstructorpattern and looks correctThe updated constructor
fn py_new(_cls: &Py<PyType>, fmt: Self::Args, vm: &VirtualMachine) -> PyResult<Self> { let spec = fmt.format_spec(vm)?; let format = fmt.0; Ok(Self { spec, format }) }
- Uses
&Py<PyType>and returnsPyResult<Self>, consistent with other modules (e.g.json,select).- Parses the format once, stores the resulting
FormatSpecand originalPyStrRef, and cleanly propagates parse errors.- Keeps integration with
#[pyclass(with(Constructor, Representable))]idiomatic.No further changes needed; this aligns with the refactor direction.
If you ever want a slightly clearer binding, you could destructure
fmtaslet IntoStructFormatBytes(format) = fmt;and useformatinstead offmt.0, but this is purely stylistic. As per coding guidelines, the macro-based constructor wiring looks good.crates/vm/src/stdlib/thread.rs (1)
204-210: Consider updating RLock and Local for consistency.While
Locknow uses theConstructortrait with the newpy_newsignature pattern (line 175),RLockandLocalstill use the olderslot_newpattern withPyTypeRefand.into_ref_with_type(vm, cls).map(Into::into). For codebase uniformity, these could be migrated to the same pattern, though this may be intentionally deferred as part of an incremental refactoring effort.Also applies to: 392-398
crates/vm/src/stdlib/typevar.rs (2)
536-615: Inconsistent with the newpy_newpattern.The
ParamSpecconstructor still has all construction logic inslot_newwith anunreachable!()stub forpy_new. For consistency withTypeVar(and the PR objective), consider moving the argument parsing and struct creation intopy_new:impl Constructor for ParamSpec { type Args = FuncArgs; fn slot_new(cls: PyTypeRef, args: Self::Args, vm: &VirtualMachine) -> PyResult { - let mut kwargs = args.kwargs; - // Parse arguments manually - let name = if args.args.is_empty() { - // ... all the argument parsing logic ... - }; - // ... rest of construction ... - let paramspec = Self { - name, - bound, - default_value, - evaluate_default: PyMutex::new(vm.ctx.none()), - covariant, - contravariant, - infer_variance, - }; - let obj = paramspec.into_ref_with_type(vm, cls)?; + let paramspec = <Self as Constructor>::py_new(&cls, args, vm)?; + let obj = paramspec.into_ref_with_type(vm, cls)?; let obj_ref: PyObjectRef = obj.into(); set_module_from_caller(&obj_ref, vm)?; Ok(obj_ref) } fn py_new(_cls: &Py<PyType>, args: Self::Args, vm: &VirtualMachine) -> PyResult<Self> { - unreachable!("use slot_new") + // Move all the argument parsing logic here + // Return Ok(Self { ... }) } }
722-777: Same inconsistency asParamSpec.
TypeVarTuplealso has all construction logic inslot_newwith anunreachable!()py_new. For consistency across all type parameter types, consider refactoring to match theTypeVarpattern.crates/vm/src/stdlib/ctypes/union.rs (2)
32-48: PyCUnionType::slot_new wiring looks correct; consider makingpy_newfail gracefully instead of panickingThe new
slot_newcorrectly delegates toPyType::slot_new, then processes_fields_only when defined directly on the new class, which matches the intended metaclass behavior. The error path when the result isn’t aPyTypeis also fine.Two small follow‑ups:
- The comment above
new_classstill mentionsPyType::py_newwhile you now callPyType::slot_new; might be worth updating for clarity.fn py_new(...) -> PyResult<Self>beingunreachable!("use slot_new")means any accidental use ofpy_newthrough theConstructortrait or future macro changes will hard‑panic the VM instead of raising a Python error. If there’s any realistic chance of that path being hit, prefer a Python‑level error instead, e.g.:- fn py_new(_cls: &Py<PyType>, _args: Self::Args, _vm: &VirtualMachine) -> PyResult<Self> { - unreachable!("use slot_new") - } + fn py_new(_cls: &Py<PyType>, _args: Self::Args, vm: &VirtualMachine) -> PyResult<Self> { + Err(vm.new_type_error("UnionType uses slot_new; py_new should not be called".to_owned())) + }If you’re confident the trait plumbing never calls
py_newfor this type, keepingunreachable!is OK, but it’d be good to double‑check that assumption against theConstructortrait and its call sites. As per coding guidelines, also make surecargo clippyis clean for this change.Also applies to: 50-52
151-191: PyCUnion::slot_new preserves union layout logic; mirrorpy_newhandling with a Python error instead ofunreachable!The new
slot_newkeeps the previous behavior: it inspects_fields_, computesmax_size/max_alignviaPyCUnionType::get_field_size, and initializes the backing buffer accordingly before wrapping withinto_ref_with_type(vm, cls). That looks consistent with the existing ctypes union semantics.A couple of low‑impact suggestions:
- As with
PyCUnionType,fn py_new(...)beingunreachable!("use slot_new")will hard‑panic if it ever gets called via theConstructortrait or macro‑generated paths. If you want a safer failure mode, you can mirror the pattern of returning a PythonTypeErrorinstead:- fn py_new(_cls: &Py<PyType>, _args: Self::Args, _vm: &VirtualMachine) -> PyResult<Self> { - unreachable!("use slot_new") - } + fn py_new(_cls: &Py<PyType>, _args: Self::Args, vm: &VirtualMachine) -> PyResult<Self> { + Err(vm.new_type_error("Union instances are created via slot_new; py_new should not be called".to_owned())) + }
slot_newignoresFuncArgs; this matches the previous implementation but is worth a quick sanity check against CPython/reference behavior if you ever plan to support initialization arguments.Please verify that no remaining call sites rely on
PyCUnion::py_newto avoid unexpected panics.Also applies to: 193-195
crates/vm/src/stdlib/typing.rs (2)
76-87:NoDefaultsingleton construction viaslot_newlooks correctUsing
type Args = ()withargs.bind(vm)?enforces a zero‑argument constructor, andslot_newreturningvm.ctx.typing_no_default.clone().into()correctly centralizes the singleton instance. Makingpy_newunreachable clearly encodes the invariant that construction must go throughslot_new; if you ever discover a Python-visible path that can hitpy_new, you may consider returning a PythonTypeErrorinstead of panicking, but that's optional.
133-172:TypeAliasType::py_newnow returningSelfmatches the newConstructorpatternThe updated signature
fn py_new(_cls: &Py<PyType>, args: Self::Args, vm: &VirtualMachine) -> PyResult<Self>and finalOk(Self::new(name, type_params, value))keep the existing validation and error messages intact while aligning with the new “py_newreturnsSelf” convention; the wrapping into a Python object can now be handled uniformly by the trait’sslot_new. The use ofSelf::Args = FuncArgsremains consistent with the existing manual parsing ofargs.crates/vm/src/stdlib/ctypes/structure.rs (2)
28-31: slot_new wiring looks correct; comment still mentionspy_newThe switch to
PyType::slot_new(metatype, args, vm)?is the right adaptation for the new constructor API, and the subsequent downcast +_fields_processing still looks sound. Minor nit: the comment on Line 29 still says “using PyType::py_new”.Consider updating it for clarity:
- // 1. Create the new class using PyType::py_new + // 1. Create the new class using PyType::slot_new
246-336: slot_new refactor for PyCStructure looks good; consider stricter arg validationRenaming this constructor entry point to
fn slot_new(cls: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResultand returningOk(py_instance.into())is consistent with the newConstructorAPI and with howwith(Constructor)is expected to drive__new__. Allocation viaStgInfoandCDataObject::from_stg_info, plus subsequent initialization usingargs.kwargsandargs.args, all look logically correct.One behavioral detail: kwargs are only applied if
fields_map.contains_key(key.as_str()), and extra positional arguments are silently ignored (if i < field_names.len()). If the goal is to closely match CPythonctypes.Structure, you may want to reject unknown field names and too many positional arguments with aTypeErrorinstead of dropping them silently, to avoid hard‑to‑spot bugs in user code.
py_newbeing anunreachable!("use slot_new")stub for this type is fine as long as no generic code path invokesConstructor::py_newforPyCStructure.crates/vm/src/builtins/tuple.rs (1)
113-152: Tupleslot_new/py_newimplementation preserves CPython-like semanticsThe new
slot_newcorrectly:
- Binds arguments via
OptionalArg<PyObjectRef>.- Fast-paths exact
tupleby returning an existingPyTupleinstance or the empty-tuple singleton.- Falls back to building
elementsand delegating topy_new, withinto_ref_with_type(vm, cls)preserving subclass semantics and the empty-tuple singleton optimization for exacttuple.This aligns with other builtins’ constructor patterns and keeps
tuple()/tuple(iterable)behavior intact.If you want to micro-tidy later, the two separate empty-tuple branches for exact
tuplecould be consolidated to reduce duplication, but it’s not necessary.crates/stdlib/src/select.rs (1)
549-551: PyEpollpy_newnow cleanly returnsSelfwith sensible input validationThe updated constructor:
- Uses the new
py_new(_cls: &Py<PyType>, ...) -> PyResult<Self>shape.- Validates
sizehint(permitting-1and positive values) and restrictsflagsto{0, EPOLL_CLOEXEC}before delegating toSelf::new().- Maps
std::io::ErrorviaIntoPyException, which keeps error handling consistent with the rest of the stdlib.No correctness issues stand out. If desired, the
sizehintcheck could be rewritten as a simple comparison (if sizehint < -1 || sizehint == 0) for readability, but the current pattern is logically fine.Also applies to: 576-587
crates/vm/src/stdlib/ctypes/array.rs (1)
58-58: Nice consistency: explicitPyCArray::int_to_bytescall.This call site (in
PyCArrayType::Callable) now explicitly usesPyCArray::int_to_bytes, matching the change at line 290 inslot_new. This improves code clarity by making the target type explicit.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
crates/stdlib/src/array.rs(3 hunks)crates/stdlib/src/bz2.rs(4 hunks)crates/stdlib/src/contextvars.rs(4 hunks)crates/stdlib/src/csv.rs(1 hunks)crates/stdlib/src/json.rs(3 hunks)crates/stdlib/src/lzma.rs(5 hunks)crates/stdlib/src/mmap.rs(6 hunks)crates/stdlib/src/overlapped.rs(3 hunks)crates/stdlib/src/pystruct.rs(2 hunks)crates/stdlib/src/select.rs(2 hunks)crates/stdlib/src/sqlite.rs(6 hunks)crates/stdlib/src/ssl.rs(6 hunks)crates/stdlib/src/zlib.rs(3 hunks)crates/vm/src/builtins/bool.rs(3 hunks)crates/vm/src/builtins/bytes.rs(2 hunks)crates/vm/src/builtins/classmethod.rs(3 hunks)crates/vm/src/builtins/code.rs(3 hunks)crates/vm/src/builtins/complex.rs(4 hunks)crates/vm/src/builtins/enumerate.rs(1 hunks)crates/vm/src/builtins/filter.rs(1 hunks)crates/vm/src/builtins/float.rs(2 hunks)crates/vm/src/builtins/function.rs(5 hunks)crates/vm/src/builtins/genericalias.rs(2 hunks)crates/vm/src/builtins/int.rs(3 hunks)crates/vm/src/builtins/list.rs(1 hunks)crates/vm/src/builtins/map.rs(1 hunks)crates/vm/src/builtins/mappingproxy.rs(1 hunks)crates/vm/src/builtins/memory.rs(1 hunks)crates/vm/src/builtins/object.rs(2 hunks)crates/vm/src/builtins/property.rs(3 hunks)crates/vm/src/builtins/set.rs(2 hunks)crates/vm/src/builtins/singletons.rs(3 hunks)crates/vm/src/builtins/slice.rs(1 hunks)crates/vm/src/builtins/staticmethod.rs(2 hunks)crates/vm/src/builtins/str.rs(2 hunks)crates/vm/src/builtins/super.rs(1 hunks)crates/vm/src/builtins/traceback.rs(2 hunks)crates/vm/src/builtins/tuple.rs(2 hunks)crates/vm/src/builtins/type.rs(2 hunks)crates/vm/src/builtins/weakproxy.rs(2 hunks)crates/vm/src/builtins/weakref.rs(2 hunks)crates/vm/src/builtins/zip.rs(2 hunks)crates/vm/src/exceptions.rs(1 hunks)crates/vm/src/protocol/object.rs(3 hunks)crates/vm/src/stdlib/ast/python.rs(2 hunks)crates/vm/src/stdlib/collections.rs(3 hunks)crates/vm/src/stdlib/ctypes/array.rs(4 hunks)crates/vm/src/stdlib/ctypes/base.rs(7 hunks)crates/vm/src/stdlib/ctypes/function.rs(5 hunks)crates/vm/src/stdlib/ctypes/pointer.rs(3 hunks)crates/vm/src/stdlib/ctypes/structure.rs(4 hunks)crates/vm/src/stdlib/ctypes/union.rs(4 hunks)crates/vm/src/stdlib/functools.rs(2 hunks)crates/vm/src/stdlib/io.rs(2 hunks)crates/vm/src/stdlib/itertools.rs(26 hunks)crates/vm/src/stdlib/operator.rs(5 hunks)crates/vm/src/stdlib/posix.rs(2 hunks)crates/vm/src/stdlib/thread.rs(2 hunks)crates/vm/src/stdlib/typevar.rs(10 hunks)crates/vm/src/stdlib/typing.rs(4 hunks)crates/vm/src/types/slot.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/stdlib/src/csv.rs
- crates/vm/src/protocol/object.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/builtins/genericalias.rs
- crates/vm/src/builtins/map.rs
- crates/stdlib/src/json.rs
- crates/vm/src/builtins/classmethod.rs
- crates/stdlib/src/bz2.rs
- crates/vm/src/stdlib/ctypes/function.rs
- crates/vm/src/builtins/super.rs
- crates/vm/src/stdlib/collections.rs
- crates/vm/src/builtins/filter.rs
- crates/vm/src/builtins/property.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/builtins/object.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/staticmethod.rscrates/vm/src/builtins/type.rscrates/vm/src/builtins/tuple.rscrates/vm/src/builtins/bool.rscrates/vm/src/exceptions.rscrates/vm/src/builtins/int.rscrates/stdlib/src/mmap.rscrates/vm/src/builtins/enumerate.rscrates/vm/src/builtins/traceback.rscrates/vm/src/builtins/weakproxy.rscrates/vm/src/builtins/slice.rscrates/vm/src/stdlib/thread.rscrates/vm/src/stdlib/io.rscrates/vm/src/stdlib/operator.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/builtins/memory.rscrates/vm/src/stdlib/posix.rscrates/stdlib/src/pystruct.rscrates/vm/src/stdlib/ctypes/union.rscrates/vm/src/stdlib/ast/python.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/stdlib/typing.rscrates/stdlib/src/ssl.rscrates/vm/src/builtins/zip.rscrates/vm/src/builtins/singletons.rscrates/vm/src/builtins/mappingproxy.rscrates/vm/src/stdlib/functools.rscrates/stdlib/src/contextvars.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/list.rscrates/vm/src/stdlib/ctypes/structure.rscrates/vm/src/types/slot.rscrates/vm/src/stdlib/ctypes/array.rscrates/stdlib/src/array.rscrates/stdlib/src/lzma.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/set.rscrates/stdlib/src/select.rscrates/stdlib/src/overlapped.rscrates/stdlib/src/zlib.rscrates/stdlib/src/sqlite.rscrates/vm/src/stdlib/typevar.rscrates/vm/src/builtins/code.rscrates/vm/src/stdlib/itertools.rs
🧠 Learnings (4)
📚 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/weakref.rscrates/vm/src/builtins/tuple.rscrates/vm/src/builtins/traceback.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/stdlib/src/pystruct.rscrates/vm/src/stdlib/ast/python.rscrates/vm/src/stdlib/typing.rscrates/stdlib/src/ssl.rscrates/vm/src/builtins/singletons.rscrates/vm/src/builtins/function.rscrates/stdlib/src/lzma.rscrates/vm/src/stdlib/typevar.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/exceptions.rscrates/stdlib/src/lzma.rscrates/stdlib/src/overlapped.rscrates/vm/src/stdlib/itertools.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 Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
crates/vm/src/stdlib/ast/python.rs
📚 Learning: 2025-08-09T22:56:24.527Z
Learnt from: jackoconnordev
Repo: RustPython/RustPython PR: 6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.527Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.
Applied to files:
crates/vm/src/stdlib/itertools.rs
🧬 Code graph analysis (33)
crates/vm/src/builtins/weakref.rs (4)
crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)
crates/vm/src/builtins/bytes.rs (3)
crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/staticmethod.rs (1)
slot_new(46-61)
crates/vm/src/builtins/type.rs (8)
crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/float.rs (2)
slot_new(134-147)py_new(149-161)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (2)
slot_new(35-111)py_new(113-115)crates/vm/src/builtins/singletons.rs (1)
slot_new(42-45)
crates/vm/src/builtins/tuple.rs (4)
crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/bool.rs (3)
crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)
crates/vm/src/exceptions.rs (5)
crates/vm/src/builtins/complex.rs (3)
slot_new(154-167)class(34-36)py_new(169-227)crates/vm/src/builtins/int.rs (3)
slot_new(210-225)class(52-54)py_new(227-247)crates/vm/src/builtins/bytes.rs (3)
slot_new(97-100)class(84-86)class(698-700)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)crates/vm/src/builtins/slice.rs (1)
slot_new(67-91)
crates/vm/src/builtins/int.rs (3)
crates/vm/src/builtins/tuple.rs (3)
slot_new(115-146)py_new(148-152)from(519-521)crates/vm/src/builtins/complex.rs (4)
slot_new(154-167)py_new(169-227)from(46-48)value(102-102)crates/vm/src/builtins/float.rs (3)
slot_new(134-147)py_new(149-161)from(54-56)
crates/stdlib/src/mmap.rs (7)
crates/vm/src/builtins/range.rs (1)
PyInt(389-390)crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/overlapped.rs (1)
py_new(267-299)crates/vm/src/builtins/genericalias.rs (1)
args(102-102)crates/vm/src/stdlib/posix.rs (1)
args(1640-1647)crates/vm/src/stdlib/io.rs (1)
vm(3478-3480)
crates/vm/src/builtins/traceback.rs (2)
crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/stdlib/src/contextvars.rs (3)
py_new(247-249)py_new(510-512)py_new(587-589)
crates/vm/src/builtins/slice.rs (2)
crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/singletons.rs (2)
slot_new(42-45)slot_new(96-99)
crates/vm/src/stdlib/io.rs (6)
crates/vm/src/stdlib/ast/other.rs (1)
object(39-39)crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)crates/stdlib/src/json.rs (1)
py_new(36-64)
crates/vm/src/stdlib/operator.rs (5)
crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/vm/src/builtins/complex.rs (1)
py_new(169-227)crates/vm/src/builtins/int.rs (1)
py_new(227-247)crates/stdlib/src/json.rs (1)
py_new(36-64)
crates/vm/src/stdlib/ctypes/pointer.rs (6)
crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/stdlib/ctypes/array.rs (3)
slot_new(252-305)py_new(78-80)py_new(307-309)crates/vm/src/stdlib/ctypes/base.rs (2)
slot_new(613-662)py_new(664-666)crates/vm/src/stdlib/ctypes/structure.rs (4)
slot_new(28-44)slot_new(246-332)py_new(46-48)py_new(334-336)
crates/vm/src/builtins/memory.rs (2)
crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)
crates/vm/src/stdlib/posix.rs (2)
crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/vm/src/builtins/float.rs (1)
py_new(149-161)
crates/stdlib/src/pystruct.rs (2)
crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/select.rs (1)
py_new(579-587)
crates/vm/src/stdlib/ctypes/base.rs (2)
crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/stdlib/ctypes/pointer.rs (2)
slot_new(79-90)py_new(92-94)
crates/vm/src/stdlib/typing.rs (2)
crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)
crates/stdlib/src/ssl.rs (3)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/vm/src/builtins/float.rs (1)
slot_new(134-147)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/zip.rs (3)
crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/vm/src/builtins/int.rs (1)
py_new(227-247)
crates/vm/src/builtins/singletons.rs (5)
crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bool.rs (1)
slot_new(104-115)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/mappingproxy.rs (2)
crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)
crates/vm/src/stdlib/functools.rs (1)
crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)
crates/stdlib/src/contextvars.rs (4)
crates/vm/src/builtins/tuple.rs (1)
slot_new(115-146)crates/vm/src/builtins/float.rs (1)
slot_new(134-147)crates/vm/src/builtins/int.rs (1)
slot_new(210-225)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/vm/src/builtins/function.rs (10)
crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/vm/src/builtins/float.rs (1)
py_new(149-161)crates/vm/src/builtins/int.rs (1)
py_new(227-247)crates/stdlib/src/array.rs (2)
py_new(654-706)value(1016-1016)crates/vm/src/builtins/code.rs (8)
args(395-404)args(407-416)args(419-428)args(431-440)args(458-466)new(29-37)new(335-337)new(1035-1037)crates/vm/src/stdlib/ctypes/function.rs (2)
args(302-302)args(366-376)crates/vm/src/stdlib/functools.rs (4)
args(67-69)args(130-130)func(62-64)func(223-223)crates/vm/src/builtins/staticmethod.rs (1)
new(69-73)crates/vm/src/stdlib/ctypes/base.rs (1)
value(54-56)
crates/vm/src/builtins/complex.rs (2)
crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/float.rs (2)
slot_new(134-147)py_new(149-161)
crates/vm/src/types/slot.rs (2)
crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)
crates/vm/src/stdlib/ctypes/array.rs (2)
crates/vm/src/stdlib/ctypes/base.rs (2)
py_new(664-666)slot_new(613-662)crates/vm/src/stdlib/ctypes/pointer.rs (2)
py_new(92-94)slot_new(79-90)
crates/stdlib/src/lzma.rs (5)
crates/stdlib/src/array.rs (1)
py_new(654-706)crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/json.rs (1)
py_new(36-64)crates/stdlib/src/mmap.rs (3)
py_new(391-490)py_new(493-638)args(913-933)crates/vm/src/stdlib/io.rs (1)
vm(3478-3480)
crates/vm/src/builtins/float.rs (2)
crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)
crates/vm/src/builtins/set.rs (6)
crates/vm/src/builtins/tuple.rs (3)
slot_new(115-146)input(122-122)py_new(148-152)crates/vm/src/builtins/complex.rs (2)
slot_new(154-167)py_new(169-227)crates/vm/src/builtins/float.rs (2)
slot_new(134-147)py_new(149-161)crates/vm/src/builtins/int.rs (2)
slot_new(210-225)py_new(227-247)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)crates/vm/src/builtins/object.rs (1)
slot_new(35-111)
crates/stdlib/src/select.rs (2)
crates/stdlib/src/bz2.rs (2)
py_new(64-68)py_new(133-154)crates/stdlib/src/csv.rs (1)
py_new(71-73)
crates/vm/src/builtins/code.rs (3)
crates/vm/src/builtins/set.rs (1)
py_new(956-958)crates/vm/src/builtins/tuple.rs (1)
py_new(148-152)crates/vm/src/builtins/complex.rs (1)
py_new(169-227)
⏰ 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). (9)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/builtins/set.rs (1)
920-957:PyFrozenSetconstructor refactor preserves semantics and adds good fast pathsThe new
Constructorimpl forPyFrozenSetlooks solid:
- Binding
FuncArgsintoOptionalArg<PyObjectRef>inslot_newmatches the Pythonfrozenset(iterable=())signature and enforces correct arg counts.- Exact-type optimizations are well-scoped:
- For
cls is frozensetand an exactPyFrozenSetargument, returning the same instance is safe and compatible with Python’s semantics (identity is an optimization, not a guarantee).- Using
vm.ctx.empty_frozensetboth forfrozenset()andfrozenset(<empty-iterable>)whenclsis the builtin type matches CPython’s behavior while still giving subclasses fresh instances viapy_new.type Args = Vec<PyObjectRef>plusfn py_new(_cls: &Py<PyType>, elements: Self::Args, vm: &VirtualMachine)nicely separates argument binding (inslot_new) from payload construction (from_iter), fitting the PR’s newpy_newpattern.Minor optional polish: you could store
let is_exact = cls.is(vm.ctx.types.frozenset_type);once and reuse it in both places (the top exact-type block and the later empty-elements check) to avoid repeating thecls.is(...)call, but that’s purely cosmetic.Overall, the constructor changes read clearly and should integrate well with the new
PyType::py_newscheme.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/vm/src/builtins/complex.rs(4 hunks)crates/vm/src/builtins/float.rs(2 hunks)crates/vm/src/builtins/int.rs(3 hunks)crates/vm/src/builtins/set.rs(2 hunks)crates/vm/src/builtins/tuple.rs(2 hunks)
🧰 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/int.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/tuple.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/set.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/tuple.rs
🧬 Code graph analysis (4)
crates/vm/src/builtins/int.rs (2)
crates/vm/src/builtins/complex.rs (4)
slot_new(154-167)py_new(169-227)from(46-48)value(102-102)crates/vm/src/builtins/float.rs (3)
slot_new(134-147)py_new(149-161)from(54-56)
crates/vm/src/builtins/complex.rs (2)
crates/vm/src/builtins/float.rs (3)
slot_new(134-147)py_new(149-161)from(54-56)crates/vm/src/builtins/int.rs (3)
slot_new(210-227)py_new(229-249)from(45-47)
crates/vm/src/builtins/tuple.rs (3)
crates/vm/src/builtins/float.rs (2)
slot_new(134-147)py_new(149-161)crates/vm/src/builtins/int.rs (2)
slot_new(210-227)py_new(229-249)crates/vm/src/builtins/bytes.rs (1)
slot_new(97-100)
crates/vm/src/builtins/float.rs (2)
crates/vm/src/builtins/complex.rs (3)
slot_new(154-167)py_new(169-227)val(176-176)crates/vm/src/builtins/int.rs (2)
slot_new(210-227)py_new(229-249)
⏰ 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). (8)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (10)
crates/vm/src/builtins/complex.rs (3)
9-12: ImportingFuncArgsis consistent with the new constructor patternBringing
FuncArgsinto scope here is exactly what’s needed for theslot_newsignature and keeps this file aligned with theint/floatconstructor implementations.
154-167: slot_new fast‑path mirrors int/float and preserves subclass semanticsThe new
slot_new:
- Uses
FuncArgs+Self::Args = ComplexArgsbinding in the same way asint/float.- Adds a fast‑path that returns the original object only when both
clsand the single arg are the exact builtincomplextype and there are no kwargs, so subclasses still get a fresh instance as in CPython.- Defers type-wrapping to
into_ref_with_type(vm, cls), which keeps subclass allocation semantics intact.This looks correct and is a nice micro-optimization.
169-227: py_new now returningSelfcleanly separates value construction from object wrappingChanging
py_newtofn py_new(_cls: &Py<PyType>, args: Self::Args, vm: &VirtualMachine) -> PyResult<Self>and:
- Returning
Ok(Self::from(Complex64 { re, im }))in the string path, and- Returning
Ok(Self::from(value))for the general case,matches the updated
Constructorpattern (value construction inpy_new, type/heap wrapping inslot_new). Ignoring_clshere is fine since all type handling occurs inslot_new, and the behavior ofcomplex()for all argument shapes is preserved.crates/vm/src/builtins/tuple.rs (2)
115-146: LGTM! Solid optimization strategy.The refactoring correctly implements the slot_new/py_new split pattern with multiple optimization paths:
- Returns exact tuple directly when input is already an exact PyTuple
- Returns empty tuple singleton when appropriate (checked at three points)
The optimization logic is sound and aligns with the pattern used in int.rs and float.rs.
148-152: LGTM! Clean payload construction.The refactored
py_newcorrectly returnsSelfdirectly, with type wrapping delegated toslot_new. The unused parameters (_cls,_vm) are intentional as part of theConstructortrait signature.crates/vm/src/builtins/int.rs (2)
210-227: LGTM! Efficient constructor optimization.The refactored
slot_newcorrectly implements:
- Guard against unsafe bool construction
- Optimization to return exact int directly when constructing
int(int_instance)- Proper argument binding and payload wrapping
The optimization avoids unnecessary allocation and is consistent with the pattern in float.rs and complex.rs.
229-249: LGTM! Refactoring correctly separates concerns.The refactored
py_newnow returnsSelf::from(value)directly instead of handling type wrapping viaSelf::with_value(cls, value, vm). This correctly delegates type management toslot_new, while preserving all the int construction logic (base handling, value extraction).crates/vm/src/builtins/float.rs (2)
134-147: LGTM! Consistent optimization pattern.The refactored
slot_newcorrectly implements the exact-float optimization to avoid unnecessary allocation when constructingfloat(float_instance). The pattern is identical to int.rs and complex.rs, ensuring consistency across numeric types.
149-161: LGTM! Clean separation of payload construction.The refactored
py_newreturnsSelf::from(float_val)directly, properly delegating type wrapping toslot_new. All float construction logic (handling missing args, type conversion, string parsing) is preserved.crates/vm/src/builtins/set.rs (1)
15-15: ImportingFuncArgsfor constructor binding looks correct
FuncArgsis now used inPyFrozenSet::slot_new, and the import is consistent with the rest of the file’s function-arg usage. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/builtins/set.rs (1)
920-959: LGTM: Well-structured slot_new/py_new refactoring.The constructor refactoring correctly implements the new contract where
py_newreturnsPyResult<Self>andslot_newhandles type-specific optimizations:
- Exact type fast-paths (lines 927-939): Efficiently returns input frozensets as-is and leverages the empty singleton for missing iterables.
- Empty singleton optimization (lines 948-950): Correctly handles the case where the iterable was present but empty, ensuring consistent singleton usage.
- Proper separation of concerns:
py_newcreates the payload viafrom_iter, whileslot_newhandles type wrapping and optimizations.The logic correctly handles subclass construction by bypassing exact-type optimizations and applying the subclass type via
into_ref_with_type.Optional: Consider addressing or removing the FIXME comment.
The FIXME comment at line 101 in
from_itermentions "empty set check". Sinceslot_newnow handles the empty frozenset case by returning the singleton directly (lines 948-950), the FIXME might be outdated. Consider either:
- Removing the FIXME if the optimization is now handled at the slot_new level.
- Clarifying the FIXME to explain any remaining concerns about empty set handling within
from_iter.crates/vm/src/builtins/int.rs (1)
210-246: slot_new implementation works but differs from sibling patterns.The implementation correctly handles all construction logic, including the bool guard, optimization path, and value extraction. However, unlike
float.rsandtuple.rs(from the relevant snippets), this implementation keeps all logic inslot_newrather than delegating topy_new.Both approaches are valid, but consider whether consistency across types would aid maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/vm/src/builtins/complex.rs(4 hunks)crates/vm/src/builtins/float.rs(2 hunks)crates/vm/src/builtins/int.rs(3 hunks)crates/vm/src/builtins/set.rs(2 hunks)crates/vm/src/builtins/tuple.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/builtins/complex.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/tuple.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/set.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/tuple.rs
🧬 Code graph analysis (2)
crates/vm/src/builtins/float.rs (4)
crates/vm/src/builtins/set.rs (2)
slot_new(923-954)py_new(956-958)crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/complex.rs (3)
slot_new(154-167)py_new(169-227)val(176-176)crates/vm/src/builtins/int.rs (3)
slot_new(210-246)args(217-217)py_new(248-250)
crates/vm/src/builtins/int.rs (4)
crates/vm/src/builtins/tuple.rs (2)
slot_new(115-146)py_new(148-152)crates/vm/src/builtins/complex.rs (3)
slot_new(154-167)value(102-102)py_new(169-227)crates/vm/src/builtins/float.rs (2)
slot_new(134-147)py_new(149-161)crates/stdlib/src/array.rs (1)
value(1016-1016)
⏰ 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). (8)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (12)
crates/vm/src/builtins/set.rs (1)
15-15: LGTM: Import addition for slot_new implementation.The
FuncArgsimport is correctly added to support the newslot_newimplementation.crates/vm/src/builtins/int.rs (4)
15-15: LGTM - Import addition supports Args type change.The FuncArgs import is necessary for the refactored constructor pattern.
208-208: Args type changed to support slot_new pattern.The change from
IntOptionstoFuncArgsis part of the refactor whereslot_newnow receives unparsed arguments and handles binding internally.
215-224: Optimization logic is well-designed.The early-return optimization correctly handles both exact int types and int subclasses:
- For exact int arguments when constructing exact int type, returns the argument as-is (zero-cost)
- For int subclass arguments, extracts the value and creates a new exact int
The logic properly falls through to the regular construction path for non-int arguments (e.g., strings) or when constructing subclasses.
248-250: Verify thatpy_newis not called directly onPyInt.The
py_newmethod panics withunreachable!(), which is a deliberate sentinel pattern used throughout the codebase for types where construction is entirely handled byslot_new. This pattern is found in numerous types (exceptions, singletons, bool, bytes, str, type, etc.) and serves to document that all initialization logic remains inslot_new.Confirm that no code paths attempt to call
Constructor::py_new()directly forPyInt, as this would cause a runtime panic. No direct calls were found in the codebase.crates/vm/src/builtins/float.rs (3)
10-14: Import ofFuncArgsis correct and necessary
FuncArgsis now used inslot_new, and the added import cleanly matches the existingfunction::{...}grouping. No issues here.
134-147:slot_newfast‑path and delegation pattern look correctThe new
slot_newimplementation:
- Mirrors the patterns used for
int,complex,tuple, andfrozenset(exact‑type fast‑path plus delegation topy_new).- Correctly restricts the reuse optimization to
floatbeing both the constructed type and the argument’s exact class, which aligns with CPython’s behavior and avoids affecting subclasses.- Uses
into_ref_with_type(vm, cls)so subclasses still get properly typed instances with the computedPyFloatpayload.No functional or style issues spotted.
149-161:py_newrefactor to returnSelfmaintains behaviorThe updated
py_new:
- Properly switches to
fn py_new(_cls: &Py<PyType>, arg: Self::Args, vm: &VirtualMachine) -> PyResult<Self>, matching the newConstructorcontract.- Keeps the previous conversion logic (empty →
0.0,try_float_optfirst, thenfloat_from_string), just returning aPyFloatpayload instead of a wrapped ref.- Ignores
_cls, which is fine since type‑dependent concerns are handled inslot_new/into_ref_with_type.Looks good and consistent with the rest of the refactor.
crates/vm/src/builtins/tuple.rs (4)
11-11: LGTM!The addition of
FuncArgsto the imports is necessary for the newslot_newsignature and follows the expected pattern for the refactoring.
148-152: LGTM! Clean implementation of the new py_new pattern.The function correctly:
- Takes
&Py<PyType>as the first parameter (aligns with PR objective)- Returns
PyResult<Self>as expected- Converts elements to boxed slice for efficient, immutable storage
The unused parameters
_clsand_vmare acceptable for trait consistency and may be used by other type implementations.
113-113: Internal refactoring is safe—no external API impact.The change from
OptionalArg<PyObjectRef>toVec<PyObjectRef>forArgsis an internal implementation detail. Thepy_newmethod is private (nopubkeyword), and the public interface remains unchanged through the customslot_newimplementation, which continues to handleOptionalArgdirectly. No external callers invokepy_newor depend on theArgstype.
115-146: Excellent optimizations for tuple construction!The slot_new implementation includes smart fast-paths:
- Returns exact PyTuple instances directly when constructing the base tuple type (avoiding unnecessary allocations)
- Returns the empty_tuple singleton for empty cases (memory efficient)
- Only creates new tuple instances when necessary
The logic correctly handles:
- Missing iterable → empty singleton (lines 128-130)
- Exact PyTuple input → return as-is (lines 121-125)
- Present but empty iterable → empty singleton (lines 139-142)
- All other cases → proper construction (lines 144-146)
2fcb831 to
a8beb45
Compare
Summary by CodeRabbit
Refactor
Chores
User impact
Tests/Docs
✏️ Tip: You can customize this high-level summary in your review settings.