flag: DISALLOW_INSTANTIATION #6445
Conversation
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)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
force-pushed
the
DISALLOW_INSTANTIATION
branch
2 times, most recently
from
bfea9b8 to
877fd89
Compare
Signed-off-by: snowapril <[email protected]>
…exist Signed-off-by: snowapril <[email protected]>
… impl Signed-off-by: snowapril <[email protected]>
youknowone
force-pushed
the
DISALLOW_INSTANTIATION
branch
2 times, most recently
from
b12575f to
41d21e9
Compare
youknowone
force-pushed
the
DISALLOW_INSTANTIATION
branch
from
41d21e9 to
2e71cd7
Compare
|
@Snowapril Hi, long time no see. This patch is superseding #5286. Could you please review this patch if you don't mind? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/derive-impl/src/pyclass.rs (1)
1697-1697: Consider removing unused variable.The
has_constructorvariable is computed but only suppressed withlet _ = 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.crates/vm/src/builtins/type.rs (1)
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() {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_descr.pyis excluded by!Lib/**
📒 Files selected for processing (29)
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)
💤 Files with no reviewable changes (1)
- crates/vm/src/types/slot.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/class.rscrates/vm/src/protocol/buffer.rscrates/stdlib/src/unicodedata.rscrates/vm/src/builtins/memory.rscrates/vm/src/builtins/frame.rscrates/stdlib/src/pystruct.rscrates/vm/src/builtins/tuple.rscrates/stdlib/src/zlib.rscrates/vm/src/builtins/generator.rscrates/vm/src/builtins/set.rscrates/stdlib/src/sqlite.rscrates/stdlib/src/csv.rscrates/vm/src/builtins/asyncgenerator.rscrates/stdlib/src/array.rscrates/vm/src/stdlib/ctypes/field.rscrates/derive-impl/src/pyclass.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/bytearray.rscrates/vm/src/stdlib/os.rscrates/vm/src/builtins/getset.rscrates/vm/src/builtins/type.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/coroutine.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/str.rscrates/vm/src/builtins/builtin_func.rscrates/vm/src/builtins/dict.rs
🧠 Learnings (5)
📚 Learning: 2025-08-26T05:20:54.540Z
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.rscrates/vm/src/builtins/bytes.rs
📚 Learning: 2025-08-26T05:20:54.540Z
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.rscrates/vm/src/builtins/bytes.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/pystruct.rscrates/vm/src/builtins/descriptor.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/stdlib/src/zlib.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/builtins/set.rscrates/stdlib/src/array.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/list.rs
🧬 Code graph analysis (3)
crates/vm/src/class.rs (1)
crates/vm/src/builtins/type.rs (3)
slot_new(1067-1363)class(148-150)__new__(559-569)
crates/vm/src/stdlib/os.rs (2)
crates/derive-impl/src/lib.rs (1)
pyclass(41-47)crates/derive/src/lib.rs (1)
pyclass(124-128)
crates/vm/src/builtins/type.rs (2)
crates/vm/src/types/slot.rs (3)
obj(1343-1344)slot_new(903-907)slot_new(931-933)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). (1)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (57)
crates/stdlib/src/unicodedata.rs (1)
108-109: LGTM!The addition of
flags(DISALLOW_INSTANTIATION)correctly prevents Python-side instantiation of theUCDclass while preserving internal Rust construction viaUcd::new(). This aligns with the broader refactor replacing theUnconstructibletrait.crates/vm/src/builtins/tuple.rs (2)
17-19: LGTM!The removal of
Unconstructiblefrom the import list is consistent with the broader refactor. The type is no longer needed sinceDISALLOW_INSTANTIATIONflag replaces it.
536-537: LGTM!The
PyTupleIteratorclass declaration correctly usesflags(DISALLOW_INSTANTIATION)instead of the previousUnconstructibletrait. The iterator retains itsIterNextandIterablecapabilities while preventing direct Python-side instantiation.crates/vm/src/class.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 fromobject. The function pointer comparison viaas usizeis a valid technique for detecting inherited slots. The special case forobjectitself ensures it retains__new__in its dict.This complements the
DISALLOW_INSTANTIATIONflag by ensuring classes that inheritslot_newfromobject(without defining their ownConstructor) don't get a redundant__new__attribute.crates/stdlib/src/array.rs (1)
1402-1403: LGTM!The
DISALLOW_INSTANTIATIONflag is correctly added toPyArrayIterwhile preserving the existingHAS_DICTflag. This prevents direct Python-side instantiation of the array iterator while maintaining its iteration capabilities.crates/vm/src/builtins/frame.rs (2)
11-12: LGTM!The import change correctly removes
Unconstructiblesince it's replaced by theDISALLOW_INSTANTIATIONflag approach.
32-33: LGTM!The
Frameclass declaration correctly usesflags(DISALLOW_INSTANTIATION)instead ofUnconstructiblein thewith()clause. Thewith(Py)is preserved for the separatePy<Frame>impl block, maintaining the existing functionality.crates/stdlib/src/csv.rs (2)
911-912: LGTM!The
Readerclass correctly addsDISALLOW_INSTANTIATIONwhile preservingIterNextandIterabletraits. This matches CPython's behavior where csv readers are created viacsv.reader()factory function rather than direct instantiation.
1062-1063: LGTM!The
Writerclass correctly addsDISALLOW_INSTANTIATION. This matches CPython's behavior where csv writers are created viacsv.writer()factory function rather than direct instantiation.crates/vm/src/builtins/getset.rs (2)
10-11: LGTM!The import correctly removes
Unconstructiblesince it's replaced by theDISALLOW_INSTANTIATIONflag.
99-100: LGTM!The
PyGetSetclass declaration correctly usesflags(DISALLOW_INSTANTIATION)instead of includingUnconstructiblein thewith()clause. TheGetDescriptortrait is preserved for descriptor protocol support.crates/vm/src/builtins/memory.rs (2)
25-27: LGTM!The import correctly removes
Unconstructiblefrom the types list since it's replaced by theDISALLOW_INSTANTIATIONflag approach.
1135-1136: LGTM!The
PyMemoryViewIteratorclass declaration correctly usesflags(DISALLOW_INSTANTIATION)instead of includingUnconstructiblein thewith()clause. TheIterNextandIterabletraits are preserved for iteration support.crates/stdlib/src/pystruct.rs (2)
19-19: LGTM!Import correctly updated to remove
Unconstructiblewhile 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/vm/src/builtins/asyncgenerator.rs (2)
11-11: LGTM!Import correctly updated to remove
Unconstructiblewhile retainingSelfIterand other necessary traits.
35-35: LGTM!The
PyAsyncGenclass correctly migrated to useflags(DISALLOW_INSTANTIATION)instead ofwith(Unconstructible), preserving thePyRefandRepresentablebehavior.crates/stdlib/src/zlib.rs (2)
228-228: LGTM!Adding
flags(DISALLOW_INSTANTIATION)toPyDecompressis correct since instances are created via thedecompressobj()factory function rather than direct instantiation.
386-386: LGTM!Adding
flags(DISALLOW_INSTANTIATION)toPyCompressis correct since instances are created via thecompressobj()factory function rather than direct instantiation.crates/vm/src/builtins/generator.rs (2)
13-13: LGTM!Import correctly updated to remove
Unconstructiblewhile retaining necessary traits.
29-29: LGTM!The
PyGeneratorclass correctly migrated to useflags(DISALLOW_INSTANTIATION)instead ofwith(Unconstructible). This aligns with CPython's behavior where generator objects cannot be directly instantiated.crates/vm/src/builtins/str.rs (2)
23-24: LGTM!Import correctly updated to remove
Unconstructiblefrom the types module import.
285-285: LGTM!The
PyStrIteratorclass correctly migrated to useflags(DISALLOW_INSTANTIATION)instead ofwith(Unconstructible). String iterators are created via the__iter__method, not direct instantiation.crates/vm/src/builtins/coroutine.rs (2)
9-9: LGTM!Import correctly updated to remove
Unconstructiblewhile retaining necessary traits.
27-27: LGTM!The
PyCoroutineclass correctly migrated to useflags(DISALLOW_INSTANTIATION)instead ofwith(Unconstructible). This aligns with CPython's behavior where coroutine objects cannot be directly instantiated.crates/derive-impl/src/pyclass.rs (3)
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 bothSLOT_NEWandslot_newnaming conventions.
1642-1644: LGTM!Correct change to only recognize
Constructorfor setting thehas_constructorflag. This aligns with the PR's shift away fromUnconstructibletoward explicitDISALLOW_INSTANTIATIONflag usage.crates/vm/src/protocol/buffer.rs (1)
398-409: VecBuffer correctly marked as non-instantiable pyclassSwitching VecBuffer from Unconstructible to
flags(BASETYPE, DISALLOW_INSTANTIATION)preserves the intended “Rust-only construction” while keeping the existingtakeAPI and buffer wiring intact. No issues spotted.crates/vm/src/builtins/set.rs (2)
19-22: Import list now matches iterator traits actually in useRemoving
Unconstructiblefrom thetypesimport is consistent with the move toDISALLOW_INSTANTIATIONand there are no remaining references in this file.
1287-1335: PySetIterator correctly transitioned to DISALLOW_INSTANTIATIONThe iterator now uses
flags(DISALLOW_INSTANTIATION)withIterNext/Iterable, keeping it non-instantiable from Python while leaving construction viaPySetInner::iter()and iteration semantics unchanged.crates/stdlib/src/sqlite.rs (3)
76-79: types import updated to match new class mixinsDropping
Unconstructibleand 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 flagMarking
Blobwithflags(DISALLOW_INSTANTIATION)(and keeping the AsMapping/AsNumber/AsSequence mixins) preserves its behavior as a connection-created handle while correctly blockingsqlite3.Blob(...)construction from Python. Internal creation inConnection::blobopenis unaffected.
2575-2647: sqlite3.Statement correctly gated behind DISALLOW_INSTANTIATION
Statementis now flaggedDISALLOW_INSTANTIATIONand continues to be created solely throughStatement::newfrom Connection/Cursor code paths. This matches prior “unconstructible” intent with no behavioral change for users.crates/vm/src/builtins/list.rs (3)
16-19: types imports trimmed to match list iterator traitsRemoving
Unconstructibleand retainingIterNext,Iterable,SelfIter, etc. matches how the list iterators are now declared and used in this module.
534-577: PyListIterator moved from Unconstructible to DISALLOW_INSTANTIATIONThe list iterator is now non-instantiable via
flags(DISALLOW_INSTANTIATION)while continuing to provideIterNext/Iterableand pickle helpers (__length_hint__,__setstate__,__reduce__). Construction throughPyList::iterremains the only path, as intended.
579-622: PyListReverseIterator now also uses DISALLOW_INSTANTIATIONThe reverse list iterator mirrors
PyListIteratorin usingDISALLOW_INSTANTIATIONplusIterNext/Iterable, keeping it internal tolist.__reversed__while leaving iteration and pickling logic untouched.crates/vm/src/stdlib/ctypes/field.rs (2)
1-4: ctypes field type imports aligned with trait usageLimiting the
typesimport toGetDescriptorandRepresentablereflects the actual traits implemented forPyCFieldafter droppingUnconstructible.
183-187: PyCField remains an immutable, non-instantiable descriptorKeeping
flags(DISALLOW_INSTANTIATION, IMMUTABLETYPE)and narrowing the mixins toRepresentableandGetDescriptorpreservesCField’s descriptor semantics while removing the now-redundantUnconstructiblehook.crates/vm/src/builtins/bytes.rs (2)
25-29: bytes.rs types import updated for iterator refactorThe
typesimport now matches current usage (IterNext, Iterable, SelfIter, etc.) and dropsUnconstructible, consistent with the new flag-based instantiation control.
739-787: PyBytesIterator correctly switched to DISALLOW_INSTANTIATIONMarking the bytes iterator with
DISALLOW_INSTANTIATIONand keepingIterNext/Iterableensures users can only obtain it viabytes.__iter__, mirroring the pattern used for list and set iterators. Iterator behavior and pickling helpers stay the same.crates/vm/src/stdlib/os.rs (3)
168-171: _os types import cleaned up for new iterator/entry traitsThe
_osmodule now imports onlyIterNext,Iterable,PyStructSequence,Representable, andSelfIter, which aligns with the traits used byDirEntry,ScandirIterator, and the struct-sequence types after Unconstructible removal.
553-759: DirEntry now flagged as non-instantiable from Python
DirEntryis correctly marked withDISALLOW_INSTANTIATIONand continues to rely on theRepresentableimpl for__repr__. All instances are still produced viascandir, so this remains a strictly runtime-created type, as in CPython.
761-787: ScandirIterator uses DISALLOW_INSTANTIATION while preserving iterator APIThe scandir iterator is now non-instantiable from Python (
DISALLOW_INSTANTIATION) and still exposesIterNext/Iterableplusclose, context manager methods, andSelfIter. Construction continues to go throughos.scandir, so behavior is preserved.crates/vm/src/builtins/range.rs (1)
13-14: LGTM! Consistent migration fromUnconstructibletrait toDISALLOW_INSTANTIATIONflag.The changes correctly:
- Remove the unused
Unconstructibleimport- Apply
flags(DISALLOW_INSTANTIATION)to bothPyLongRangeIteratorandPyRangeIterator- Retain the necessary
IterNextandIterabletraits along withSelfIterimplementationsAlso applies to: 551-551, 616-616
crates/vm/src/builtins/bytearray.rs (1)
35-36: LGTM! Clean migration forPyByteArrayIterator.The
Unconstructibletrait is properly replaced withflags(DISALLOW_INSTANTIATION)while retaining the essentialIterNextandIterabletraits for iterator functionality.Also applies to: 868-868
crates/vm/src/builtins/type.rs (3)
1743-1781: Solid implementation of CPython'stp_new_wrappersafety checks.The implementation correctly:
- Checks
DISALLOW_INSTANTIATIONflag on the subtype being instantiated- Implements the "is not safe" check by finding the first non-heap base (
staticbase)- Compares
tp_newslots betweentypandstaticbaseto prevent unsafe instantiation patterns likeobject.__new__(dict)- Uses the MRO lookup to find and invoke the actual
slot_newThis matches CPython's behavior for protecting against improper
__new__usage.
1594-1618: LGTM! Clean integration with the new slot-based instantiation control.The
Callable::callimplementation correctly:
- Short-circuits
type(x)calls for efficiency- Checks
slots.new.load()and produces an appropriate error if None (which happens whenDISALLOW_INSTANTIATIONis set viaset_new)- Maintains the subclass check before calling
__init__
194-194: LGTM! Proper slot propagation for both heap and static types.The changes ensure:
HEAPTYPEflag is set early innew_heapbefore other processingset_newis called at the end ofinit_slots(for heap types) and after creation innew_static- Both paths correctly propagate the
tp_newslot from base classes when not disallowedAlso applies to: 201-202, 407-408, 441-443
crates/vm/src/builtins/descriptor.rs (1)
7-7: LGTM! Descriptors correctly marked as non-instantiable.Both
PyMethodDescriptorandPyMemberDescriptorare properly updated:
DISALLOW_INSTANTIATIONflag added alongside existing flags (METHOD_DESCRIPTOR,BASETYPE)- Essential traits (
GetDescriptor,Callable,Representable) retainedUnconstructibleimport removedAlso applies to: 107-110, 247-250
crates/vm/src/builtins/builtin_func.rs (1)
8-8: LGTM! Native function types correctly updated.Both
PyNativeFunctionandPyNativeMethodare properly migrated:
DISALLOW_INSTANTIATIONadded to prevent Python-side instantiationHAS_DICTflag preserved for dict-backed instances- Essential traits (
Callable,Comparable,Representable) retainedAlso applies to: 77-77, 155-158
crates/vm/src/builtins/dict.rs (6)
21-22: LGTM - Import cleanup is correct.The
Unconstructibletrait is properly removed from the imports since it's no longer used in this file after switching to theDISALLOW_INSTANTIATIONflag.
851-851: LGTM - Macro iterator declaration correctly updated.The
flags(DISALLOW_INSTANTIATION)properly replaces theUnconstructibletrait while preservingIterNextandIterablefunctionality for the forward iterators generated by the macro.
924-924: LGTM - Reverse iterator declaration correctly updated.Consistent with the forward iterator changes -
DISALLOW_INSTANTIATIONflag replacesUnconstructiblefor 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, andRepresentable.
1190-1201: LGTM - PyDictItems declaration correctly updated.Consistent with
PyDictKeys- the pyclass usesflags(DISALLOW_INSTANTIATION)and retains all necessary traits for dict_items behavior including set-like operations viaViewSetOpsandComparable.
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 ofComparable,ViewSetOps, andAsNumberis intentional and correct sincedict_valuesdoesn't support set-like operations in CPython (values may be unhashable).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/derive-impl/src/pyclass.rs(2 hunks)crates/vm/src/builtins/type.rs(5 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/derive-impl/src/pyclass.rscrates/vm/src/builtins/type.rs
🧬 Code graph analysis (1)
crates/vm/src/builtins/type.rs (2)
crates/vm/src/types/slot.rs (3)
obj(1343-1344)slot_new(903-907)slot_new(931-933)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). (11)
- 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)
🔇 Additional comments (7)
crates/derive-impl/src/pyclass.rs (2)
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 likeSLOT_NEWexist 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.crates/vm/src/builtins/type.rs (5)
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
slotsparameter is necessary for this modification.
372-408: LGTM: Consistent new-slot propagation for static types.The call to
set_newafter type creation ensures that static types properly inherit or respect theDISALLOW_INSTANTIATIONflag and new-slot behavior. The mutableslotsparameter is consistent with thenew_heapsignature.
1578-1608: Inconsistent error message formatting for disallowed instantiation.The
callimplementation useszelf.slots.nameto construct the error message, whilecall_slot_newusessubtype.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
HEAPTYPEflag, or when there's no parent base (explicitbreakat line 1748). While Python semantics guarantee that all types ultimately inherit fromobject(a non-heap type), the code correctly handles the edge case.Function pointer comparison: The pattern
fn as usizecomparison is used consistently throughout the codebase (e.g.,crates/vm/src/vm/vm_ops.rsuses an identical comparison pattern for slots). While Rust'sOption<fn>supports direct==comparison, theas usizeapproach 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_newfunction correctly ensures that theDISALLOW_INSTANTIATIONflag takes unconditional precedence. When the flag is set (line 446),slots.newis unconditionally stored asNone, clearing any pre-existing value. This is the first condition checked, ensuring the flag overrides any inherited or existing slot. Additionally, a runtime check incall_slot_new(line 1736) provides redundant protection by explicitly rejecting instantiation with an error when the flag is set. Types withDISALLOW_INSTANTIATIONwill never have a callable new slot.
| 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.
🧩 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 Agents
In 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.
|
@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, 😄 |
|
Seems also good to me :) |
|
@Snowapril @ShaharNaveh Thanks! |
3 participants
close #5286
close #3692
close #4455
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.