flag: DISALLOW_INSTANTIATION - #6445
Conversation
Sorry, something went wrong.
WalkthroughReplaces the Unconstructible trait with a new DISALLOW_INSTANTIATION PyTypeFlags across many pyclass declarations, removes the Unconstructible trait, refines pyclass derive slot/Constructor handling, and updates type initialization and new binding to propagate new-slot behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Derive as pyclass derive
participant TypeSys as type creation (new_heap/new_static)
participant ClassInit as class initialization (class.rs)
note right of Derive: Macro resolves pyclass attrs\n(normalize slot names, detect Constructor)
Derive->>TypeSys: emit PyTypeSlots (with/without new slot)
TypeSys->>TypeSys: set_new(slots, base) -- mutate slots,tp_new propagation
TypeSys->>ClassInit: register slots on type
ClassInit->>ClassInit: if own slot_new then bind __new__
ClassInit-->>TypeSys: final type ready with DISALLOW_INSTANTIATION flag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Signed-off-by: snowapril <sinjihng@gmail.com>
…exist Signed-off-by: snowapril <sinjihng@gmail.com>
… impl Signed-off-by: snowapril <sinjihng@gmail.com>
|
@Snowapril Hi, long time no see. This patch is superseding #5286. Could you please review this patch if you don't mind? |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)crates/derive-impl/src/pyclass.rs (1)📜 Review detailscrates/vm/src/builtins/type.rs (1)1697-1697: Consider removing unused variable.
The has_constructor variable is computed but only suppressed with let _ = has_constructor;. If this is intentional placeholder for future logic, consider adding a TODO comment. Otherwise, consider removing the variable and related tracking code to reduce dead code.
445-466: Remove commented-out dead code.
The commented-out block (lines 446-455) appears to be exploratory code that was superseded by the current implementation. Dead code clutters the codebase and can cause confusion.
fn set_new(slots: &PyTypeSlots, base: &Option<PyTypeRef>) { - // if self.slots.new.load().is_none() - // && self - // .base - // .as_ref() - // .map(|base| base.class().is(ctx.types.object_type)) - // .unwrap_or(false) - // && self.slots.flags.contains(PyTypeFlags::HEAPTYPE) - // { - // self.slots.flags |= PyTypeFlags::DISALLOW_INSTANTIATION; - // } - if slots.flags.contains(PyTypeFlags::DISALLOW_INSTANTIATION) { slots.new.store(None) } else if slots.new.load().is_none() {
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 272b36d and 2e71cd7.
⛔ Files ignored due to path filters (1)- Lib/test/test_descr.py is excluded by !Lib/**
- crates/derive-impl/src/pyclass.rs (3 hunks)
- crates/stdlib/src/array.rs (1 hunks)
- crates/stdlib/src/csv.rs (2 hunks)
- crates/stdlib/src/pystruct.rs (2 hunks)
- crates/stdlib/src/sqlite.rs (3 hunks)
- crates/stdlib/src/unicodedata.rs (1 hunks)
- crates/stdlib/src/zlib.rs (2 hunks)
- crates/vm/src/builtins/asyncgenerator.rs (2 hunks)
- crates/vm/src/builtins/builtin_func.rs (3 hunks)
- crates/vm/src/builtins/bytearray.rs (2 hunks)
- crates/vm/src/builtins/bytes.rs (2 hunks)
- crates/vm/src/builtins/coroutine.rs (2 hunks)
- crates/vm/src/builtins/descriptor.rs (3 hunks)
- crates/vm/src/builtins/dict.rs (6 hunks)
- crates/vm/src/builtins/frame.rs (2 hunks)
- crates/vm/src/builtins/generator.rs (2 hunks)
- crates/vm/src/builtins/getset.rs (2 hunks)
- crates/vm/src/builtins/list.rs (3 hunks)
- crates/vm/src/builtins/memory.rs (2 hunks)
- crates/vm/src/builtins/range.rs (3 hunks)
- crates/vm/src/builtins/set.rs (2 hunks)
- crates/vm/src/builtins/str.rs (2 hunks)
- crates/vm/src/builtins/tuple.rs (2 hunks)
- crates/vm/src/builtins/type.rs (5 hunks)
- crates/vm/src/class.rs (1 hunks)
- crates/vm/src/protocol/buffer.rs (1 hunks)
- crates/vm/src/stdlib/ctypes/field.rs (2 hunks)
- crates/vm/src/stdlib/os.rs (3 hunks)
- crates/vm/src/types/slot.rs (0 hunks)
- crates/vm/src/types/slot.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/vm/src/class.rs
- crates/vm/src/protocol/buffer.rs
- crates/stdlib/src/unicodedata.rs
- crates/vm/src/builtins/memory.rs
- crates/vm/src/builtins/frame.rs
- crates/stdlib/src/pystruct.rs
- crates/vm/src/builtins/tuple.rs
- crates/stdlib/src/zlib.rs
- crates/vm/src/builtins/generator.rs
- crates/vm/src/builtins/set.rs
- crates/stdlib/src/sqlite.rs
- crates/stdlib/src/csv.rs
- crates/vm/src/builtins/asyncgenerator.rs
- crates/stdlib/src/array.rs
- crates/vm/src/stdlib/ctypes/field.rs
- crates/derive-impl/src/pyclass.rs
- crates/vm/src/builtins/range.rs
- crates/vm/src/builtins/bytearray.rs
- crates/vm/src/stdlib/os.rs
- crates/vm/src/builtins/getset.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/builtins/bytes.rs
- crates/vm/src/builtins/coroutine.rs
- crates/vm/src/builtins/descriptor.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/builtins/builtin_func.rs
- crates/vm/src/builtins/dict.rs
Learnt from: youknowone Repo: RustPython/RustPython PR: 6110 File: vm/src/frame.rs:1311-1316 Timestamp: 2025-08-26T05:20:54.540Z Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Applied to files:
- crates/vm/src/builtins/memory.rs
- crates/vm/src/builtins/bytes.rs
Learnt from: youknowone Repo: RustPython/RustPython PR: 6110 File: vm/src/frame.rs:1311-1316 Timestamp: 2025-08-26T05:20:54.540Z Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Applied to files:
- crates/vm/src/builtins/memory.rs
- crates/vm/src/builtins/bytes.rs
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
- crates/stdlib/src/pystruct.rs
- crates/vm/src/builtins/descriptor.rs
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to 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/stdlib/src/zlib.rs
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/builtins/set.rs
- crates/stdlib/src/array.rs
- crates/vm/src/builtins/range.rs
- crates/vm/src/builtins/bytes.rs
- crates/vm/src/builtins/list.rs
crates/vm/src/builtins/type.rs (3)crates/vm/src/stdlib/os.rs (2)
- slot_new (1067-1363)
- class (148-150)
- __new__ (559-569)
crates/derive-impl/src/lib.rs (1)crates/vm/src/builtins/type.rs (2)crates/derive/src/lib.rs (1)
- pyclass (41-47)
- pyclass (124-128)
crates/vm/src/types/slot.rs (3)⏰ 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). (1)crates/vm/src/builtins/object.rs (1)
- obj (1343-1344)
- slot_new (903-907)
- slot_new (931-933)
- slot_new (35-111)
- GitHub Check: Run snippets and cpython tests (windows-latest)
crates/stdlib/src/unicodedata.rs (1)crates/vm/src/builtins/tuple.rs (2)108-109: LGTM!
The addition of flags(DISALLOW_INSTANTIATION) correctly prevents Python-side instantiation of the UCD class while preserving internal Rust construction via Ucd::new(). This aligns with the broader refactor replacing the Unconstructible trait.
crates/vm/src/class.rs (1)17-19: LGTM!
The removal of Unconstructible from the import list is consistent with the broader refactor. The type is no longer needed since DISALLOW_INSTANTIATION flag replaces it.
536-537: LGTM!
The PyTupleIterator class declaration correctly uses flags(DISALLOW_INSTANTIATION) instead of the previous Unconstructible trait. The iterator retains its IterNext and Iterable capabilities while preventing direct Python-side instantiation.
crates/stdlib/src/array.rs (1)119-136: Conditional __new__ binding logic looks correct.
The implementation properly avoids adding __new__ to a class's __dict__ when the slot is inherited from object. The function pointer comparison via as usize is a valid technique for detecting inherited slots. The special case for object itself ensures it retains __new__ in its dict.
This complements the DISALLOW_INSTANTIATION flag by ensuring classes that inherit slot_new from object (without defining their own Constructor) don't get a redundant __new__ attribute.
crates/vm/src/builtins/frame.rs (2)1402-1403: LGTM!
The DISALLOW_INSTANTIATION flag is correctly added to PyArrayIter while preserving the existing HAS_DICT flag. This prevents direct Python-side instantiation of the array iterator while maintaining its iteration capabilities.
crates/stdlib/src/csv.rs (2)11-12: LGTM!
The import change correctly removes Unconstructible since it's replaced by the DISALLOW_INSTANTIATION flag approach.
32-33: LGTM!
The Frame class declaration correctly uses flags(DISALLOW_INSTANTIATION) instead of Unconstructible in the with() clause. The with(Py) is preserved for the separate Py<Frame> impl block, maintaining the existing functionality.
crates/vm/src/builtins/getset.rs (2)911-912: LGTM!
The Reader class correctly adds DISALLOW_INSTANTIATION while preserving IterNext and Iterable traits. This matches CPython's behavior where csv readers are created via csv.reader() factory function rather than direct instantiation.
1062-1063: LGTM!
The Writer class correctly adds DISALLOW_INSTANTIATION. This matches CPython's behavior where csv writers are created via csv.writer() factory function rather than direct instantiation.
crates/vm/src/builtins/memory.rs (2)10-11: LGTM!
The import correctly removes Unconstructible since it's replaced by the DISALLOW_INSTANTIATION flag.
99-100: LGTM!
The PyGetSet class declaration correctly uses flags(DISALLOW_INSTANTIATION) instead of including Unconstructible in the with() clause. The GetDescriptor trait is preserved for descriptor protocol support.
crates/stdlib/src/pystruct.rs (2)25-27: LGTM!
The import correctly removes Unconstructible from the types list since it's replaced by the DISALLOW_INSTANTIATION flag approach.
1135-1136: LGTM!
The PyMemoryViewIterator class declaration correctly uses flags(DISALLOW_INSTANTIATION) instead of including Unconstructible in the with() clause. The IterNext and Iterable traits are preserved for iteration support.
crates/vm/src/builtins/asyncgenerator.rs (2)19-19: LGTM!
Import correctly updated to remove Unconstructible while retaining necessary traits for iterator functionality.
192-199: LGTM!
The migration from #[pyclass(with(Unconstructible, IterNext, Iterable))] to #[pyclass(with(IterNext, Iterable), flags(DISALLOW_INSTANTIATION))] correctly implements the new instantiation control mechanism while preserving iterator behavior.
crates/stdlib/src/zlib.rs (2)11-11: LGTM!
Import correctly updated to remove Unconstructible while retaining SelfIter and other necessary traits.
35-35: LGTM!
The PyAsyncGen class correctly migrated to use flags(DISALLOW_INSTANTIATION) instead of with(Unconstructible), preserving the PyRef and Representable behavior.
crates/vm/src/builtins/generator.rs (2)228-228: LGTM!
Adding flags(DISALLOW_INSTANTIATION) to PyDecompress is correct since instances are created via the decompressobj() factory function rather than direct instantiation.
386-386: LGTM!
Adding flags(DISALLOW_INSTANTIATION) to PyCompress is correct since instances are created via the compressobj() factory function rather than direct instantiation.
crates/vm/src/builtins/str.rs (2)13-13: LGTM!
Import correctly updated to remove Unconstructible while retaining necessary traits.
29-29: LGTM!
The PyGenerator class correctly migrated to use flags(DISALLOW_INSTANTIATION) instead of with(Unconstructible). This aligns with CPython's behavior where generator objects cannot be directly instantiated.
crates/vm/src/builtins/coroutine.rs (2)23-24: LGTM!
Import correctly updated to remove Unconstructible from the types module import.
285-285: LGTM!
The PyStrIterator class correctly migrated to use flags(DISALLOW_INSTANTIATION) instead of with(Unconstructible). String iterators are created via the __iter__ method, not direct instantiation.
crates/derive-impl/src/pyclass.rs (3)9-9: LGTM!
Import correctly updated to remove Unconstructible while retaining necessary traits.
27-27: LGTM!
The PyCoroutine class correctly migrated to use flags(DISALLOW_INSTANTIATION) instead of with(Unconstructible). This aligns with CPython's behavior where coroutine objects cannot be directly instantiated.
crates/vm/src/protocol/buffer.rs (1)957-960: LGTM!
Good improvement to the error message to clarify that #[pyslot] can be applied to both methods and const function pointers.
1502-1508: LGTM!
Good robustness improvement. Converting to lowercase before stripping the slot_ prefix enables consistent handling of both SLOT_NEW and slot_new naming conventions.
1642-1644: LGTM!
Correct change to only recognize Constructor for setting the has_constructor flag. This aligns with the PR's shift away from Unconstructible toward explicit DISALLOW_INSTANTIATION flag usage.
crates/vm/src/builtins/set.rs (2)398-409: VecBuffer correctly marked as non-instantiable pyclass
Switching VecBuffer from Unconstructible to flags(BASETYPE, DISALLOW_INSTANTIATION) preserves the intended “Rust-only construction” while keeping the existing take API and buffer wiring intact. No issues spotted.
crates/stdlib/src/sqlite.rs (3)19-22: Import list now matches iterator traits actually in use
Removing Unconstructible from the types import is consistent with the move to DISALLOW_INSTANTIATION and there are no remaining references in this file.
1287-1335: PySetIterator correctly transitioned to DISALLOW_INSTANTIATION
The iterator now uses flags(DISALLOW_INSTANTIATION) with IterNext/Iterable, keeping it non-instantiable from Python while leaving construction via PySetInner::iter() and iteration semantics unchanged.
crates/vm/src/builtins/list.rs (3)76-79: types import updated to match new class mixins
Dropping Unconstructible and keeping only the traits actually mixed into SQLite types (Connection, Cursor, Row, Blob, Statement) aligns the imports with current usage.
2191-2254: sqlite3.Blob now non-instantiable from Python via flag
Marking Blob with flags(DISALLOW_INSTANTIATION) (and keeping the AsMapping/AsNumber/AsSequence mixins) preserves its behavior as a connection-created handle while correctly blocking sqlite3.Blob(...) construction from Python. Internal creation in Connection::blobopen is unaffected.
2575-2647: sqlite3.Statement correctly gated behind DISALLOW_INSTANTIATION
Statement is now flagged DISALLOW_INSTANTIATION and continues to be created solely through Statement::new from Connection/Cursor code paths. This matches prior “unconstructible” intent with no behavioral change for users.
crates/vm/src/stdlib/ctypes/field.rs (2)16-19: types imports trimmed to match list iterator traits
Removing Unconstructible and retaining IterNext, Iterable, SelfIter, etc. matches how the list iterators are now declared and used in this module.
534-577: PyListIterator moved from Unconstructible to DISALLOW_INSTANTIATION
The list iterator is now non-instantiable via flags(DISALLOW_INSTANTIATION) while continuing to provide IterNext/Iterable and pickle helpers (__length_hint__, __setstate__, __reduce__). Construction through PyList::iter remains the only path, as intended.
579-622: PyListReverseIterator now also uses DISALLOW_INSTANTIATION
The reverse list iterator mirrors PyListIterator in using DISALLOW_INSTANTIATION plus IterNext/Iterable, keeping it internal to list.__reversed__ while leaving iteration and pickling logic untouched.
crates/vm/src/builtins/bytes.rs (2)1-4: ctypes field type imports aligned with trait usage
Limiting the types import to GetDescriptor and Representable reflects the actual traits implemented for PyCField after dropping Unconstructible.
183-187: PyCField remains an immutable, non-instantiable descriptor
Keeping flags(DISALLOW_INSTANTIATION, IMMUTABLETYPE) and narrowing the mixins to Representable and GetDescriptor preserves CField’s descriptor semantics while removing the now-redundant Unconstructible hook.
crates/vm/src/stdlib/os.rs (3)25-29: bytes.rs types import updated for iterator refactor
The types import now matches current usage (IterNext, Iterable, SelfIter, etc.) and drops Unconstructible, consistent with the new flag-based instantiation control.
739-787: PyBytesIterator correctly switched to DISALLOW_INSTANTIATION
Marking the bytes iterator with DISALLOW_INSTANTIATION and keeping IterNext/Iterable ensures users can only obtain it via bytes.__iter__, mirroring the pattern used for list and set iterators. Iterator behavior and pickling helpers stay the same.
crates/vm/src/builtins/range.rs (1)168-171: _os types import cleaned up for new iterator/entry traits
The _os module now imports only IterNext, Iterable, PyStructSequence, Representable, and SelfIter, which aligns with the traits used by DirEntry, ScandirIterator, and the struct-sequence types after Unconstructible removal.
553-759: DirEntry now flagged as non-instantiable from Python
DirEntry is correctly marked with DISALLOW_INSTANTIATION and continues to rely on the Representable impl for __repr__. All instances are still produced via scandir, so this remains a strictly runtime-created type, as in CPython.
761-787: ScandirIterator uses DISALLOW_INSTANTIATION while preserving iterator API
The scandir iterator is now non-instantiable from Python (DISALLOW_INSTANTIATION) and still exposes IterNext/Iterable plus close, context manager methods, and SelfIter. Construction continues to go through os.scandir, so behavior is preserved.
crates/vm/src/builtins/bytearray.rs (1)13-14: LGTM! Consistent migration from Unconstructible trait to DISALLOW_INSTANTIATION flag.
The changes correctly:
- Remove the unused Unconstructible import
- Apply flags(DISALLOW_INSTANTIATION) to both PyLongRangeIterator and PyRangeIterator
- Retain the necessary IterNext and Iterable traits along with SelfIter implementations
Also applies to: 551-551, 616-616
crates/vm/src/builtins/type.rs (3)35-36: LGTM! Clean migration for PyByteArrayIterator.
The Unconstructible trait is properly replaced with flags(DISALLOW_INSTANTIATION) while retaining the essential IterNext and Iterable traits for iterator functionality.
Also applies to: 868-868
crates/vm/src/builtins/descriptor.rs (1)1743-1781: Solid implementation of CPython's tp_new_wrapper safety checks.
The implementation correctly:
- Checks DISALLOW_INSTANTIATION flag on the subtype being instantiated
- Implements the "is not safe" check by finding the first non-heap base (staticbase)
- Compares tp_new slots between typ and staticbase to prevent unsafe instantiation patterns like object.__new__(dict)
- Uses the MRO lookup to find and invoke the actual slot_new
This matches CPython's behavior for protecting against improper __new__ usage.
1594-1618: LGTM! Clean integration with the new slot-based instantiation control.
The Callable::call implementation correctly:
- Short-circuits type(x) calls for efficiency
- Checks slots.new.load() and produces an appropriate error if None (which happens when DISALLOW_INSTANTIATION is set via set_new)
- Maintains the subclass check before calling __init__
194-194: LGTM! Proper slot propagation for both heap and static types.
The changes ensure:
- HEAPTYPE flag is set early in new_heap before other processing
- set_new is called at the end of init_slots (for heap types) and after creation in new_static
- Both paths correctly propagate the tp_new slot from base classes when not disallowed
Also applies to: 201-202, 407-408, 441-443
crates/vm/src/builtins/builtin_func.rs (1)7-7: LGTM! Descriptors correctly marked as non-instantiable.
Both PyMethodDescriptor and PyMemberDescriptor are properly updated:
- DISALLOW_INSTANTIATION flag added alongside existing flags (METHOD_DESCRIPTOR, BASETYPE)
- Essential traits (GetDescriptor, Callable, Representable) retained
- Unconstructible import removed
Also applies to: 107-110, 247-250
crates/vm/src/builtins/dict.rs (6)8-8: LGTM! Native function types correctly updated.
Both PyNativeFunction and PyNativeMethod are properly migrated:
- DISALLOW_INSTANTIATION added to prevent Python-side instantiation
- HAS_DICT flag preserved for dict-backed instances
- Essential traits (Callable, Comparable, Representable) retained
Also applies to: 77-77, 155-158
21-22: LGTM - Import cleanup is correct.
The Unconstructible trait is properly removed from the imports since it's no longer used in this file after switching to the DISALLOW_INSTANTIATION flag.
851-851: LGTM - Macro iterator declaration correctly updated.
The flags(DISALLOW_INSTANTIATION) properly replaces the Unconstructible trait while preserving IterNext and Iterable functionality for the forward iterators generated by the macro.
924-924: LGTM - Reverse iterator declaration correctly updated.
Consistent with the forward iterator changes - DISALLOW_INSTANTIATION flag replaces Unconstructible for all reverse iterators generated by the macro.
1125-1136: LGTM - PyDictKeys declaration correctly updated.
The pyclass attribute properly uses flags(DISALLOW_INSTANTIATION) while preserving all the required traits for dict_keys behavior: DictView, Comparable, Iterable, ViewSetOps, AsSequence, AsNumber, and Representable.
1190-1201: LGTM - PyDictItems declaration correctly updated.
Consistent with PyDictKeys - the pyclass uses flags(DISALLOW_INSTANTIATION) and retains all necessary traits for dict_items behavior including set-like operations via ViewSetOps and Comparable.
1265-1268: LGTM - PyDictValues declaration correctly updated.
The pyclass correctly uses flags(DISALLOW_INSTANTIATION) with a reduced trait set (DictView, Iterable, AsSequence, Representable). The absence of Comparable, ViewSetOps, and AsNumber is intentional and correct since dict_values doesn't support set-like operations in CPython (values may be unhashable).
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 2e71cd7 and 639e89a.
📒 Files selected for processing (2)- crates/derive-impl/src/pyclass.rs (2 hunks)
- crates/vm/src/builtins/type.rs (5 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/derive-impl/src/pyclass.rs
- crates/vm/src/builtins/type.rs
crates/vm/src/types/slot.rs (3)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)crates/vm/src/builtins/object.rs (1)
- obj (1343-1344)
- slot_new (903-907)
- slot_new (931-933)
- slot_new (35-111)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- 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: Run snippets and cpython tests (windows-latest)
crates/derive-impl/src/pyclass.rs (2)crates/vm/src/builtins/type.rs (5)957-960: LGTM: Error message now accurately reflects accepted item types.
The updated error message correctly indicates that #[pyslot] can be applied to both methods and const function pointers, matching the validation logic at lines 952-956.
1502-1504: Normalize slot method names to lowercase for consistent processing.
This code converts slot method identifiers to lowercase before stripping the slot_ prefix. However, the existing codebase exclusively uses lowercase slot method names (e.g., slot_new, slot_init, slot_del). No uppercase variants like SLOT_NEW exist in the codebase, so this lowercasing is a normalization step rather than a mechanism to handle multiple naming conventions. This is consistent with the lowercasing already performed at line 967.
190-203: LGTM: Early HEAPTYPE flag initialization is appropriate.
Setting the HEAPTYPE flag immediately when creating heap types is logically sound and improves clarity. The mutable slots parameter is necessary for this modification.
372-408: LGTM: Consistent new-slot propagation for static types.
The call to set_new after type creation ensures that static types properly inherit or respect the DISALLOW_INSTANTIATION flag and new-slot behavior. The mutable slots parameter is consistent with the new_heap signature.
1578-1608: Inconsistent error message formatting for disallowed instantiation.
The call implementation uses zelf.slots.name to construct the error message, while call_slot_new uses subtype.slot_name() for the same error. Both produce "cannot create '{}' instances", but the inconsistent name retrieval method should be unified.
1726-1771: The staticbase finding logic and function pointer comparison are correct and follow established patterns in the codebase.
Staticbase loop termination: The loop safely terminates through two mechanisms: when the type no longer has HEAPTYPE flag, or when there's no parent base (explicit break at line 1748). While Python semantics guarantee that all types ultimately inherit from object (a non-heap type), the code correctly handles the edge case.
Function pointer comparison: The pattern fn as usize comparison is used consistently throughout the codebase (e.g., crates/vm/src/vm/vm_ops.rs uses an identical comparison pattern for slots). While Rust's Option<fn> supports direct == comparison, the as usize approach is valid and deterministic since function pointer addresses are stable. This aligns with CPython's own comparison approach.
No changes required.
445-455: No issues found; the implementation correctly handles the flag precedence.
The set_new function correctly ensures that the DISALLOW_INSTANTIATION flag takes unconditional precedence. When the flag is set (line 446), slots.new is unconditionally stored as None, clearing any pre-existing value. This is the first condition checked, ensuring the flag overrides any inherited or existing slot. Additionally, a runtime check in call_slot_new (line 1736) provides redundant protection by explicitly rejecting instantiation with an error when the flag is set. Types with DISALLOW_INSTANTIATION will never have a callable new slot.
Sorry, something went wrong.
| let mut staticbase = subtype.clone(); | ||
| while staticbase.slots.flags.has_feature(PyTypeFlags::HEAPTYPE) { | ||
| if let Some(base) = staticbase.base.as_ref() { | ||
| staticbase = base.clone(); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Check if staticbase's tp_new differs from typ's tp_new | ||
| let typ_new = typ.slots.new.load(); | ||
| let staticbase_new = staticbase.slots.new.load(); | ||
| if typ_new.map(|f| f as usize) != staticbase_new.map(|f| f as usize) { | ||
| return Err(vm.new_type_error(format!( | ||
| "{}.__new__({}) is not safe, use {}.__new__()", | ||
| typ.slot_name(), | ||
| subtype.slot_name(), | ||
| staticbase.slot_name() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
# Search for similar .load() patterns with type conversions
echo "=== Searching for .load() with as usize conversions ==="
rg -n "\.load\(\).*as usize" --type=rust -C 2
echo ""
echo "=== Searching for function pointer comparisons ==="
rg -n "fn.*as usize" --type=rust -C 2
echo ""
echo "=== Searching for slots.new usage pattern ==="
rg -n "slots\.new" --type=rust -C 1 | head -100
echo ""
echo "=== Searching for atomic function pointer usage ==="
rg -n "AtomicPtr.*fn\|fn.*AtomicPtr" --type=rust -C 2
echo ""
echo "=== Check the definition of slots struct ==="
rg -n "struct.*slots\|slots:" --type=rust crates/vm/src/ -B2 -A5 | head -80Repository: RustPython/RustPython
Length of output: 3289
🏁 Script executed:
#!/bin/bash
# Search for comments explaining the function pointer comparison pattern
echo "=== Search for comments near the comparison ==="
sed -n '1750,1765p' crates/vm/src/builtins/type.rs
echo ""
echo "=== Search for comments about function pointers in slots ==="
rg -n "function pointer|tp_new|slot.*new" --type=rust crates/vm/src/builtins/type.rs | head -30
echo ""
echo "=== Check git history/blame for context ==="
git log --oneline -n 20 -- crates/vm/src/builtins/type.rs | head -15
echo ""
echo "=== Look for direct function pointer comparison patterns ==="
rg -n "\.load\(\).*==|\.load\(\).*!=" --type=rust | grep -v "as usize" | head -20
echo ""
echo "=== Check PyTypeSlots definition ==="
rg -n "slots.*new\|struct.*Slots\|pub.*new:" --type=rust crates/vm/src/builtins/type.rs | head -40Repository: RustPython/RustPython
Length of output: 2522
🏁 Script executed:
#!/bin/bash
# Check how function pointers are typically compared in Rust
echo "=== Direct function pointer comparisons (if any) ==="
rg -n "\.load\(\).*==\s*\." --type=rust crates/vm/src/ | head -20
echo ""
echo "=== Check if Option<fn> implements PartialEq ==="
# This is a language feature, but let's check how comparisons are done elsewhere
rg -n "Option.*fn\|fn.*load\(\)" --type=rust crates/vm/src/builtins/type.rs -A2 | head -50
echo ""
echo "=== Look for PartialEq or comparison traits on slot functions ==="
rg -n "impl.*Eq\|impl.*PartialEq\|derive.*Eq" --type=rust crates/vm/src/ -C2 | head -40
echo ""
echo "=== Check the exact type definition for slots.new ==="
rg -n "pub.*new:" --type=rust crates/vm/src/builtins/type.rs -B5 | head -60
echo ""
echo "=== Verify that function pointers can be directly compared ==="
cat > /tmp/test_fn_cmp.rs << 'EOF'
fn foo() {}
fn bar() {}
fn main() {
let f1: Option<fn()> = Some(foo);
let f2: Option<fn()> = Some(bar);
let f3: Option<fn()> = Some(foo);
// Direct comparison
if f1 != f2 { println!("f1 != f2"); }
if f1 == f3 { println!("f1 == f3"); }
// Via as usize
if f1.map(|f| f as usize) != f2.map(|f| f as usize) { println!("via usize: f1 != f2"); }
}
EOF
echo "Rust allows direct comparison of Option<fn()>"Repository: RustPython/RustPython
Length of output: 399
Consider using direct function pointer comparison instead of casting to usize.
Line 1757 casts function pointers to usize for comparison:
if typ_new.map(|f| f as usize) != staticbase_new.map(|f| f as usize) {In Rust, Option<fn()> implements PartialEq directly and can be compared without casting:
if typ_new != staticbase_new {The cast to usize is unnecessary and less idiomatic. Direct comparison is clearer and avoids implementation-defined pointer-to-integer conversions.
🤖 Prompt for AI AgentsIn crates/vm/src/builtins/type.rs around lines 1745 to 1764, the code compares function pointers by mapping them to usize (typ_new.map(|f| f as usize) != staticbase_new.map(|f| f as usize)); replace this with a direct comparison of the Option function pointers (typ_new != staticbase_new) to use Option<fn(...)> PartialEq, removing the unnecessary cast and map; update the condition to directly compare typ_new and staticbase_new so the logic stays the same but uses idiomatic, safe pointer comparison.
Sorry, something went wrong.
|
@youknowone Thanks for notifying me. As a first step toward getting back into this project, I’d like to review this and refresh my understanding of the details, 😄 |
Sorry, something went wrong.
|
Seems also good to me :) |
Sorry, something went wrong.
close #5286
close #3692
close #4455
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.