Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
📝 WalkthroughWalkthroughThe changes refactor the builtin function and method type hierarchy to unify PyNativeFunction and PyNativeMethod under a shared "builtin_function_or_method" type with C-compatible representation, introduce a new PyCapsule type for capsule objects, and add a centralized _types module exposing all built-in Python type objects as accessor functions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/_types.rs`:
- Around line 107-111: The ClassMethodDescriptorType function currently returns
vm.ctx.types.method_descriptor_type, causing types.ClassMethodDescriptorType to
be identical to MethodDescriptorType; instead either register and return a
distinct type object for class method descriptors (e.g. create and expose a new
vm.ctx.types.class_method_descriptor_type via the same type-registration flow
used for other descriptor types and return that from ClassMethodDescriptorType)
or remove/withhold the pyattr until the dedicated type is implemented; update
the ClassMethodDescriptorType implementation to return the new unique type
symbol (class_method_descriptor_type) rather than method_descriptor_type so type
identity matches CPython.
🧹 Nitpick comments (2)
crates/vm/src/builtins/capsule.rs (1)
10-16: Consider simplifying the struct definition.Since
PyCapsuleonly contains a unit field_private: (), theCloneandCopyderives are technically fine but may be unnecessary. If capsule functionality expands in the future to hold actual data (e.g., raw pointers for FFI), these derives would need reconsideration.♻️ Optional simplification
-#[derive(Debug, Clone, Copy)] +#[derive(Debug)] pub struct PyCapsule { _private: (), }crates/vm/src/builtins/builtin_func.rs (1)
233-244: Consider adding a SAFETY comment to the unsafe block.The
downcast_uncheckedrelies on the#[repr(C)]layout invariant established in the struct definitions. Adding an explicit// SAFETY:comment would help future maintainers understand why this cast is valid.📝 Suggested documentation
if obj.fast_isinstance(class) { - // Both PyNativeFunction and PyNativeMethod share the same type now. - // PyNativeMethod has `func: PyNativeFunction` as its first field, - // so we can safely treat the data pointer as PyNativeFunction for reading. + // SAFETY: Both PyNativeFunction and PyNativeMethod share the same Python type. + // PyNativeMethod has `func: PyNativeFunction` as its first field with #[repr(C)], + // guaranteeing func is at offset 0. This allows safely reading PyNativeFunction + // fields from either type. Ok(Self(unsafe { obj.downcast_unchecked() }))
| #[pyattr] | ||
| fn ClassMethodDescriptorType(vm: &VirtualMachine) -> PyObjectRef { | ||
| // TODO: implement as separate type | ||
| vm.ctx.types.method_descriptor_type.to_owned().into() | ||
| } |
There was a problem hiding this comment.
ClassMethodDescriptorType should not alias MethodDescriptorType.
On Line 109, this TODO means types.ClassMethodDescriptorType becomes identical to types.MethodDescriptorType, which diverges from CPython and can break code that relies on type identity checks. Please introduce a dedicated type (or avoid exposing this attribute until it exists).
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/_types.rs` around lines 107 - 111, The
ClassMethodDescriptorType function currently returns
vm.ctx.types.method_descriptor_type, causing types.ClassMethodDescriptorType to
be identical to MethodDescriptorType; instead either register and return a
distinct type object for class method descriptors (e.g. create and expose a new
vm.ctx.types.class_method_descriptor_type via the same type-registration flow
used for other descriptor types and return that from ClassMethodDescriptorType)
or remove/withhold the pyattr until the dedicated type is implemented; update
the ClassMethodDescriptorType implementation to return the new unique type
symbol (class_method_descriptor_type) rather than method_descriptor_type so type
identity matches CPython.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin _types |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/reprlib.py dependencies:
dependent tests: (162 tests)
[x] lib: cpython/Lib/threading.py dependencies:
dependent tests: (109 tests)
Legend:
|
1 participant
the
_prefixed name is related to #3284Summary by CodeRabbit
Release Notes
New Features
_typesmodule providing access to built-in type objects including AsyncGeneratorType, BuiltinFunctionType, BuiltinMethodType, CapsuleType, CodeType, FunctionType, GeneratorType, MethodType, ModuleType, and others.Refactor
✏️ Tip: You can customize this high-level summary in your review settings.