Actionable comments posted: 3

crates/vm/src/stdlib/ctypes/union.rs (1)

159-171: Alignment calculation uses field size directly.

In lines 169-170, for unions, max_align is set to max_align.max(size) where size is the field size. The comment "For simple types, alignment == size" is correct for primitive C types, but for composite types (nested structures/unions), this may not hold. This could lead to incorrect alignment for complex union layouts.

Consider using a proper alignment calculation similar to PyCStructType::get_field_align if the field type is not a simple type.

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

606-613: Consider using get_type_data for has_type_data.

The current implementation manually reads the lock and checks slot presence. This duplicates logic and holds the read lock for the entire chain of checks.

A simpler implementation could be:

     pub fn has_type_data<T: Any + 'static>(&self) -> bool {
-        self.heaptype_ext.as_ref().is_some_and(|ext| {
-            ext.type_data
-                .read()
-                .as_ref()
-                .is_some_and(|slot| slot.get::<T>().is_some())
-        })
+        self.get_type_data::<T>().is_some()
     }

This reuses the existing logic and is more maintainable.

crates/vm/src/stdlib/ctypes/base.rs (2)

237-249: Consider adding debug logging for failed _type_ resolution.

When get_stg_info falls back to StgInfo::default(), the caller has no indication that the type resolution failed. This could make debugging issues harder. Consider adding a tracing or debug-level log when the fallback path is taken.

---

766-773: Alignment parameter uses element_size, which may not always be correct.

The StgInfo::new_array() call passes element_size as the alignment. While this is often correct (e.g., c_int is 4 bytes with 4-byte alignment), some types may have different alignment requirements. Consider deriving alignment from the actual type's alignment rather than assuming it equals element size.

+        // Get alignment from type if available, otherwise use element_size
+        let align = if let Ok(type_attr) = cls.as_object().get_attr("_type_", vm) {
+            if let Ok(s) = type_attr.str(vm) {
+                let s = s.to_string();
+                if s.len() == 1 {
+                    super::_ctypes::get_align(&s)
+                } else {
+                    element_size
+                }
+            } else {
+                element_size
+            }
+        } else {
+            element_size
+        };
         let stg_info = super::util::StgInfo::new_array(
             total_size,
-            element_size,
+            align,
             n as usize,
             cls.clone().into(),
             element_size,
         );

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

26-58: The type_id field is redundant with downcast_ref but provides a fast-path check.

The get<T>() method first checks type_id == TypeId::of::<T>() before calling downcast_ref(). Since downcast_ref() internally performs the same type ID check, this is slightly redundant. However, the explicit check may serve as documentation or a fast-path. If intentional, consider adding a comment explaining this is a defensive check.

     /// Get a reference to the data if the type matches.
     pub fn get<T: Any + 'static>(&self) -> Option<&T> {
+        // Fast-path type check before downcast (downcast_ref also checks internally)
         if self.type_id == TypeId::of::<T>() {
             self.data.downcast_ref()
         } else {
             None
         }
     }

crates/vm/src/stdlib/ctypes/array.rs (1)

668-758: Consider extracting common in_dll logic to reduce duplication.

Both PyCArrayType::in_dll (lines 126-211) and PyCArray::in_dll (lines 668-758) contain nearly identical code for:

• Getting the library handle
• Looking up the library in cache
• Getting the symbol address

This could be extracted into a helper function to reduce maintenance burden.

// Helper function to extract common logic
fn get_symbol_address(
    dll: &PyObjectRef,
    name: &str,
    vm: &VirtualMachine,
) -> PyResult<usize> {
    use libloading::Symbol;
    
    let handle = if let Ok(int_handle) = dll.try_int(vm) {
        int_handle
            .as_bigint()
            .to_usize()
            .ok_or_else(|| vm.new_value_error("Invalid library handle".to_owned()))?
    } else {
        dll.get_attr("_handle", vm)?
            .try_int(vm)?
            .as_bigint()
            .to_usize()
            .ok_or_else(|| vm.new_value_error("Invalid library handle".to_owned()))?
    };

    let library_cache = crate::stdlib::ctypes::library::libcache().read();
    let library = library_cache
        .get_lib(handle)
        .ok_or_else(|| vm.new_attribute_error("Library not found".to_owned()))?;

    let symbol_name = format!("{}\0", name);
    let inner_lib = library.lib.lock();

    if let Some(lib) = &*inner_lib {
        unsafe {
            let symbol: Symbol<'_, *mut u8> = lib.get(symbol_name.as_bytes()).map_err(|e| {
                vm.new_attribute_error(format!("{}: symbol '{}' not found", e, name))
            })?;
            Ok(*symbol as usize)
        }
    } else {
        Err(vm.new_attribute_error("Library is closed".to_owned()))
    }
}

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 72e892a and 4e47b6a.

• crates/vm/src/builtins/type.rs (5 hunks)
• crates/vm/src/stdlib/ctypes.rs (2 hunks)
• crates/vm/src/stdlib/ctypes/array.rs (5 hunks)
• crates/vm/src/stdlib/ctypes/base.rs (3 hunks)
• crates/vm/src/stdlib/ctypes/pointer.rs (1 hunks)
• crates/vm/src/stdlib/ctypes/structure.rs (3 hunks)
• crates/vm/src/stdlib/ctypes/union.rs (1 hunks)
• crates/vm/src/stdlib/ctypes/util.rs (3 hunks)
• crates/vm/src/types/slot.rs (2 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/vm/src/stdlib/ctypes/pointer.rs
• crates/vm/src/stdlib/ctypes/union.rs
• crates/vm/src/stdlib/ctypes/base.rs
• crates/vm/src/stdlib/ctypes.rs
• crates/vm/src/stdlib/ctypes/structure.rs
• crates/vm/src/types/slot.rs
• crates/vm/src/stdlib/ctypes/util.rs
• crates/vm/src/builtins/type.rs
• crates/vm/src/stdlib/ctypes/array.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/vm/src/stdlib/ctypes/pointer.rs

crates/vm/src/stdlib/ctypes/array.rs (2)

• __mul__ (95-123)
• create_array_type_with_stginfo (30-66)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

crates/vm/src/stdlib/ctypes/array.rs (1)

• create_array_type_with_stginfo (30-66)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

crates/vm/src/stdlib/ctypes/array.rs (1)

• create_array_type_with_stginfo (30-66)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

crates/vm/src/object/payload.rs (1)

• std (22-22)

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

• slot (611-611)

crates/vm/src/stdlib/ctypes.rs (3)

• std (418-418)
• std (444-444)
• std (696-696)

crates/vm/src/stdlib/ctypes/array.rs (5)

• std (278-278)
• std (281-281)
• std (284-284)
• fmt (248-253)
• length (87-92)

crates/vm/src/stdlib/ctypes/field.rs (1)

• size (259-261)

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

• new (34-39)
• new (191-197)
• try_new (68-72)
• try_new (91-97)
• slot (69-69)
• slot (93-93)

• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run tests under miri
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Check Rust code with clippy

crates/vm/src/stdlib/ctypes/structure.rs (2)

18-19: LGTM - Clean simplification of PyCStructType.

The empty struct with #[derive(Debug, PyPayload, Default)] is appropriate for a metaclass that stores its data via the new TypeDataSlot mechanism rather than inline fields.

---

173-180: The concern about alignment is unfounded. Examining all StgInfo::new_array calls across the ctypes module shows that passing element_size as the alignment parameter is the consistent and intentional pattern used in:

• structure.rs (lines 173-180)
• pointer.rs (lines 26-31)
• array.rs (lines 114-119)
• base.rs (lines 766-771)

In ctypes, the alignment of an array is correctly defined as the alignment of its element type. The code is correct.

Likely an incorrect or invalid review comment.

crates/vm/src/stdlib/ctypes/union.rs (1)

16-17: LGTM - Consistent with PyCStructType refactoring.

The empty struct with derives matches the pattern used in structure.rs, maintaining consistency across ctypes metaclasses.

crates/vm/src/stdlib/ctypes/pointer.rs (2)

12-13: LGTM - Consistent metaclass pattern.

The empty struct with #[derive(PyPayload, Debug, Default)] follows the same pattern as other ctypes metaclasses.

---

26-33: Pointer array creation correctly uses pointer size for alignment.

For pointer arrays, using element_size (which equals std::mem::size_of::<usize>()) as the alignment is correct since pointers are naturally aligned to their own size on all common platforms.

crates/vm/src/stdlib/ctypes.rs (2)

398-408: LGTM - TypeDataSlot-based size lookup with proper fallbacks.

The implementation correctly prioritizes TypeDataSlot on the instance's class first, then checks if the object itself is a type with TypeDataSlot, before falling back to legacy paths. This ensures backward compatibility while leveraging the new type-data infrastructure.

---

670-680: LGTM - Alignment lookup mirrors size_of pattern.

The alignment function follows the same lookup strategy as size_of, ensuring consistent behavior across both operations.

crates/vm/src/builtins/type.rs (4)

70-70: LGTM - New type_data field with appropriate locking.

Adding pub type_data: PyRwLock<Option<TypeDataSlot>> to HeapTypeExt provides thread-safe storage for per-type data. The Option wrapper allows for lazy initialization.

---

575-587: LGTM - init_type_data with proper guards.

The implementation correctly:

1. Returns an error for non-heap types
2. Prevents double-initialization by checking if data already exists
3. Uses proper write locking for the update

---

591-603: LGTM - Clean accessor pattern with guard types.

Both get_type_data and get_type_data_mut properly delegate to TypeDataRef::try_new and TypeDataRefMut::try_new respectively, which handle the type checking and guard mapping internally.

---

209-209: LGTM - Consistent initialization across heap type construction paths.

Both new_heap (line 209) and slot_new (line 1218) properly initialize type_data to PyRwLock::new(None), ensuring all heap types start with uninitialized type data.

Also applies to: 1218-1218

crates/vm/src/stdlib/ctypes/util.rs (2)

25-38: LGTM: Manual Debug implementation is correct.

The manual Debug impl properly formats all fields of StgInfo. This was necessary because the derive was removed (likely because PyObjectRef doesn't implement Debug in a useful way or at all).

---

55-87: LGTM: Constructors are well-structured.

The new() and new_array() factory methods provide clear, type-safe construction with appropriate defaults. The new_array() constructor properly captures all array-specific metadata.

crates/vm/src/stdlib/ctypes/base.rs (1)

232-232: LGTM: Empty struct aligns with new type-data architecture.

The migration from embedded stg_info field to an empty struct is consistent with the PR's goal of storing metadata via TypeDataSlot on the type object itself rather than per-instance.

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

60-112: LGTM: Guard wrappers provide safe, zero-cost type-erased access.

The TypeDataRef and TypeDataRefMut wrappers correctly use try_map to safely downcast while holding the lock, and the Deref/DerefMut implementations enable ergonomic access to the underlying data.

crates/vm/src/stdlib/ctypes/array.rs (2)

29-65: LGTM: Factory function properly creates array types with type-data storage.

The create_array_type_with_stginfo function correctly:

1. Creates a new type with PyCArrayType as metaclass and PyCArray as base
2. Attempts to initialize type-data with init_type_data
3. Falls back to updating existing type-data if already initialized

The fallback update pattern (lines 58-62) handles the case where type data was already set, which is a reasonable defensive approach.

---

78-92: LGTM: Accessors safely handle missing type-data.

The typ and length getters use chained Option operations with appropriate fallbacks (vm.ctx.none() and 0), which is consistent with Python's duck-typing semantics where attributes may be missing.

Actionable comments posted: 0

crates/vm/src/stdlib/ctypes/util.rs (1)

19-23: Address previous review: Add cfg feature gate for thread-safety bounds.

As flagged in the previous review, the unsafe impl Send and unsafe impl Sync should be conditionally gated with the threading feature to match PyObjectRef's own implementations. The comment should also clarify the actual synchronization mechanism (e.g., GIL).

-// StgInfo is stored in type_data which requires Send + Sync.
-// The PyObjectRef in proto/element_type fields is protected by the type system's locking mechanism.
-// CPython: ctypes objects are not thread-safe by design; users must synchronize access.
-unsafe impl Send for StgInfo {}
-unsafe impl Sync for StgInfo {}
+// StgInfo is stored in type_data which requires Send + Sync.
+// The PyObjectRef fields are protected by Python's GIL when threading is enabled.
+// CPython: ctypes objects are not thread-safe by design; users must synchronize access.
+#[cfg(feature = "threading")]
+unsafe impl Send for StgInfo {}
+#[cfg(feature = "threading")]
+unsafe impl Sync for StgInfo {}

crates/vm/src/stdlib/ctypes/array.rs (1)

95-123: Avoid unwrap() on downcast_ref and use StgInfo alignment instead of size in __mul__ / meta in_dll

Both __mul__ and the metaclass-level in_dll currently do:

• let type_ref = zelf.downcast_ref::<PyType>().unwrap();

If zelf is ever not a PyType (e.g. misused from Python or via an internal call), this will panic instead of raising a Python exception. Past review already pointed this out for both call sites; the issue is still present.

Additionally, __mul__ passes the inner size as the align argument to StgInfo::new_array:

let (_inner_length, inner_size) = ... stg.size ...
...
let stg_info = StgInfo::new_array(
    total_size,
    new_element_size, // <-- using size as align
    n as usize,
    current_array_type,
    new_element_size,
);

Conceptually, array alignment should come from the inner type’s alignment (stg.align), not from its total size. Using size as align will give incorrect alignment for nested arrays and any place that relies on align (e.g. libffi layouts).

A concrete fix that addresses both concerns and makes missing StgInfo a Python error could look like:

@@
     #[pymethod]
     fn __mul__(zelf: PyObjectRef, n: isize, vm: &VirtualMachine) -> PyResult {
         if n < 0 {
             return Err(vm.new_value_error(format!("Array length must be >= 0, not {n}")));
         }

-        // Get inner array info from TypeDataSlot
-        let type_ref = zelf.downcast_ref::<PyType>().unwrap();
-        let (_inner_length, inner_size) = type_ref
-            .get_type_data::<StgInfo>()
-            .map(|stg| (stg.length, stg.size))
-            .unwrap_or((0, 0));
+        // Get inner array info from type_data; require `zelf` to be a type with StgInfo
+        let type_ref = zelf.downcast_ref::<PyType>().ok_or_else(|| {
+            vm.new_type_error("descriptor '__mul__' requires a type object".to_owned())
+        })?;
+        let (inner_size, inner_align) = type_ref
+            .get_type_data::<StgInfo>()
+            .map(|stg| (stg.size, stg.align))
+            .ok_or_else(|| {
+                vm.new_type_error("array type has no storage info (StgInfo)".to_owned())
+            })?;

         // The element type of the new array is the current array type itself
         let current_array_type: PyObjectRef = zelf.clone();

-        // Element size is the total size of the inner array
-        let new_element_size = inner_size;
-        let total_size = new_element_size * (n as usize);
-
-        let stg_info = StgInfo::new_array(
-            total_size,
-            new_element_size,
-            n as usize,
-            current_array_type,
-            new_element_size,
-        );
+        // Each element is the full inner array
+        let element_size = inner_size;
+        let total_size = element_size * (n as usize);
+
+        let stg_info = StgInfo::new_array(
+            total_size,
+            inner_align,
+            n as usize,
+            current_array_type,
+            element_size,
+        );

         create_array_type_with_stginfo(stg_info, vm)
@@
-        // Get size from the array type via TypeDataSlot
-        let type_ref = zelf.downcast_ref::<PyType>().unwrap();
-        let (element_type, length, element_size) = type_ref
-            .get_type_data::<StgInfo>()
-            .map(|stg| {
-                (
-                    stg.element_type.clone().unwrap_or_else(|| vm.ctx.none()),
-                    stg.length,
-                    stg.element_size,
-                )
-            })
-            .unwrap_or_else(|| (vm.ctx.none(), 0, 0));
+        // Get size from the array type via type_data
+        let type_ref = zelf.downcast_ref::<PyType>().ok_or_else(|| {
+            vm.new_type_error("in_dll requires a type object".to_owned())
+        })?;
+        let stg = type_ref
+            .get_type_data::<StgInfo>()
+            .ok_or_else(|| {
+                vm.new_type_error("array type has no storage info (StgInfo)".to_owned())
+            })?;
+        let element_type = stg.element_type.clone().unwrap_or_else(|| vm.ctx.none());
+        let length = stg.length;
+        let element_size = stg.element_size;

This keeps the normal path unchanged for correctly constructed array types (which always have a StgInfo set via create_array_type_with_stginfo), while:

• eliminating potential panics from unwrap(), and
• using the stored alignment as intended by StgInfo::new_array.

Also applies to: 172-184

crates/vm/src/stdlib/ctypes.rs (1)

410-421: Fix duplicate comment numbering.

Lines 410 and 421 both use "// 3." numbering. Consider renumbering for clarity:

-        // 3. Instances with cdata buffer
+        // 3. Instances with cdata buffer
         if let Some(structure) = obj.downcast_ref::<PyCStructure>() {
...
-        // 3. Type objects
+        // 4. Type objects
         if let Ok(type_ref) = obj.clone().downcast::<crate::builtins::PyType>() {

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

571-612: TypeData API on PyType is well-scoped; minor simplification possible

The init_type_data / get_type_data / get_type_data_mut / has_type_data API cleanly enforces:

• heap-only storage (via heaptype_ext check),
• single initialization under a write lock, and
• typed access via TypeDataRef / TypeDataRefMut.

One small possible simplification: has_type_data could be implemented in terms of get_type_data::<T>().is_some() to avoid duplicating the TypeId/type-matching logic already encapsulated in TypeDataRef::try_new, but that’s purely cosmetic.

crates/vm/src/stdlib/ctypes/array.rs (1)

23-66: Array-type helper and meta layout look good; consider tightening error handling and naming

The refactor to make PyCArrayType empty and push layout into per-type StgInfo stored in type_data, plus:

• create_array_type_with_stginfo constructing a heap type via PyType::slot_new with PyCArrayType as metaclass, and
• _type_ / _length_ on PyCArrayType reading directly from StgInfo,

is a clean way to centralize ctypes array metadata and matches the new TypeDataSlot design.

Two non-blocking refinements you might consider:

• In create_array_type_with_stginfo, treating any Err from init_type_data as “already initialized, update in place” can hide real misuse; since this helper always creates a fresh heap type, it would be clearer to either:

  • propagate the error as a Python exception, or
  • debug_assert! that init_type_data succeeds and only fall back to mutation in explicitly reused-type scenarios.

• type_name = format!("Array_{}", stg_info.length) can collide for different element types with the same length. If the name is used only for debugging this is fine; if you rely on it for identity or error messages, encoding the element type (e.g. its name) into the string would make types more self-describing.

The AsNumber multiply wiring that calls PyCArrayType::__mul__(a.to_owned(), n, vm) also looks consistent with the metaclass-based protocol.

Also applies to: 76-92, 214-225

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 4e47b6a and 1707f96.

• crates/vm/src/builtins/type.rs (5 hunks)
• crates/vm/src/stdlib/ctypes.rs (2 hunks)
• crates/vm/src/stdlib/ctypes/array.rs (5 hunks)
• crates/vm/src/stdlib/ctypes/base.rs (3 hunks)
• crates/vm/src/stdlib/ctypes/pointer.rs (1 hunks)
• crates/vm/src/stdlib/ctypes/structure.rs (3 hunks)
• crates/vm/src/stdlib/ctypes/union.rs (1 hunks)
• crates/vm/src/stdlib/ctypes/util.rs (3 hunks)
• crates/vm/src/types/slot.rs (2 hunks)

• crates/vm/src/stdlib/ctypes/base.rs
• 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/stdlib/ctypes.rs
• crates/vm/src/stdlib/ctypes/union.rs
• crates/vm/src/stdlib/ctypes/structure.rs
• crates/vm/src/builtins/type.rs
• crates/vm/src/stdlib/ctypes/util.rs
• crates/vm/src/stdlib/ctypes/pointer.rs
• crates/vm/src/stdlib/ctypes/array.rs

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Applied to files:

• crates/vm/src/stdlib/ctypes/util.rs

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Applied to files:

• crates/vm/src/stdlib/ctypes/util.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/vm/src/stdlib/ctypes/pointer.rs

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

• new (33-38)
• new (190-196)
• try_new (67-71)
• try_new (90-96)
• slot (68-68)
• slot (92-92)

crates/vm/src/stdlib/ctypes.rs (3)

• std (418-418)
• std (444-444)
• std (696-696)

crates/vm/src/stdlib/ctypes/array.rs (1)

• create_array_type_with_stginfo (30-66)

crates/vm/src/stdlib/ctypes.rs (17)

• mem (144-144)
• mem (145-145)
• mem (146-146)
• mem (147-147)
• mem (148-148)
• mem (149-149)
• mem (150-150)
• mem (151-151)
• mem (152-152)
• mem (153-153)
• mem (154-154)
• mem (155-155)
• mem (156-156)
• mem (157-157)
• mem (158-158)
• mem (212-212)
• size_of (391-449)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

crates/vm/src/stdlib/ctypes/util.rs (3)

• new (56-67)
• default (41-52)
• new_array (70-87)

• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: auto_format
• GitHub Check: Check the WASM package and demo

crates/vm/src/stdlib/ctypes/pointer.rs (2)

12-13: LGTM: Empty struct aligns with TypeDataSlot refactor.

The removal of the stg_info field from PyCPointerType correctly shifts metadata storage to the new TypeDataSlot mechanism.

---

26-33: Verify: align set to element_size for pointer arrays.

Using element_size (pointer size) for both alignment and element size is correct for pointer types since pointer alignment equals pointer size. The refactor to use create_array_type_with_stginfo properly centralizes array type creation.

crates/vm/src/stdlib/ctypes/union.rs (1)

16-17: LGTM: Empty struct consistent with TypeDataSlot refactor.

The removal of internal state from PyCUnionType aligns with the PR's pattern of moving metadata to TypeDataSlot.

crates/vm/src/stdlib/ctypes.rs (2)

398-408: LGTM: TypeDataSlot-first approach for size resolution.

The new logic correctly prioritizes TypeDataSlot lookup for both instances (via obj.class()) and type objects (via direct downcast), with proper fallbacks to existing mechanisms.

---

670-680: LGTM: Consistent TypeDataSlot pattern for alignment.

The alignment function mirrors the size_of logic correctly, checking TypeDataSlot first before falling back to existing mechanisms.

crates/vm/src/stdlib/ctypes/structure.rs (2)

18-19: LGTM: Empty struct consistent with TypeDataSlot refactor.

The pattern matches PyCPointerType and PyCUnionType, properly shifting metadata to the new storage mechanism.

---

173-180: The current implementation correctly uses element_size for alignment, which is the consistent pattern throughout the ctypes module. Both pointer arrays (pointer.rs) and generic arrays (array.rs, base.rs) use the same approach. Additionally, Self::calculate_struct_alignment does not exist and cannot be feasibly implemented—the structure's alignment information is not accessible in this context. The design pattern of using element size as alignment appears intentional and aligns with how C language ctypes spacing works.

Likely an incorrect or invalid review comment.

crates/vm/src/stdlib/ctypes/util.rs (2)

69-87: LGTM: Clean API for array-type StgInfo construction.

Taking element_type: PyObjectRef directly (rather than Option<PyObjectRef>) is appropriate since arrays always have an element type. The internal wrapping to Some(element_type) is handled correctly.

---

40-53: LGTM: Default implementation correctly initializes new fields.

The default values (element_type: None, element_size: 0) are appropriate for non-array types.

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

63-70: HeapTypeExt type_data wiring and initialization look correct

HeapTypeExt now carries type_data: PyRwLock<Option<TypeDataSlot>>, and both heap-type construction paths (new_heap and the heap path in slot_new) initialize it to PyRwLock::new(None). That ensures:

• static types never have a type_data slot (heaptype_ext: None there), and
• heap types start with an empty slot, so init_type_data’s one-shot semantics are reliable.

No issues from a correctness or threading perspective.

Also applies to: 201-209, 1205-1219

Actionable comments posted: 1

crates/vm/src/stdlib/ctypes/util.rs (1)

19-23: Unsafe Send/Sync impls need feature gating to match PyObjectRef.

This is a known issue from a previous review. The unsafe impl Send and unsafe impl Sync for StgInfo are unconditional, but StgInfo contains PyObjectRef fields (proto, element_type) which only implement Send/Sync when the threading feature is enabled. These impls should be conditionally gated with #[cfg(feature = "threading")] to match PyObjectRef's bounds in object/core.rs. Additionally, clarify the comment to explicitly state the synchronization mechanism (e.g., the GIL).

-// StgInfo is stored in type_data which requires Send + Sync.
-// The PyObjectRef in proto/element_type fields is protected by the type system's locking mechanism.
-// CPython: ctypes objects are not thread-safe by design; users must synchronize access.
-unsafe impl Send for StgInfo {}
-unsafe impl Sync for StgInfo {}
+// StgInfo is stored in type_data which requires Send + Sync.
+// The PyObjectRef in proto/element_type fields is protected by the GIL.
+// CPython: ctypes objects are not thread-safe by design; users must synchronize access.
+#[cfg(feature = "threading")]
+unsafe impl Send for StgInfo {}
+#[cfg(feature = "threading")]
+unsafe impl Sync for StgInfo {}

crates/vm/src/stdlib/ctypes/array.rs (1)

95-123: Critical: .unwrap() can panic if zelf is not a PyType.

Line 101 calls .unwrap() after downcast_ref::<PyType>() without validation. Since zelf is PyObjectRef, this is not statically guaranteed and will panic if zelf is not a type object.

Note: This issue was flagged in previous reviews and marked as addressed, but the .unwrap() is still present in the current code.

Apply this fix to handle the downcast safely:

     fn __mul__(zelf: PyObjectRef, n: isize, vm: &VirtualMachine) -> PyResult {
         if n < 0 {
             return Err(vm.new_value_error(format!("Array length must be >= 0, not {n}")));
         }
 
         // Get inner array info from TypeDataSlot
-        let type_ref = zelf.downcast_ref::<PyType>().unwrap();
+        let type_ref = zelf.downcast_ref::<PyType>().ok_or_else(|| {
+            vm.new_type_error("descriptor '__mul__' requires a type object".to_owned())
+        })?;
         let (_inner_length, inner_size) = type_ref
             .get_type_data::<StgInfo>()
             .map(|stg| (stg.length, stg.size))
             .unwrap_or((0, 0));

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 1707f96 and 7a2b967.

• crates/vm/src/stdlib/ctypes.rs (2 hunks)
• crates/vm/src/stdlib/ctypes/array.rs (5 hunks)
• crates/vm/src/stdlib/ctypes/base.rs (3 hunks)
• crates/vm/src/stdlib/ctypes/pointer.rs (1 hunks)
• crates/vm/src/stdlib/ctypes/structure.rs (3 hunks)
• crates/vm/src/stdlib/ctypes/union.rs (1 hunks)
• crates/vm/src/stdlib/ctypes/util.rs (3 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/vm/src/stdlib/ctypes/base.rs
• crates/vm/src/stdlib/ctypes.rs
• crates/vm/src/stdlib/ctypes/util.rs
• crates/vm/src/stdlib/ctypes/union.rs
• crates/vm/src/stdlib/ctypes/structure.rs
• crates/vm/src/stdlib/ctypes/array.rs
• crates/vm/src/stdlib/ctypes/pointer.rs

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Applied to files:

• crates/vm/src/stdlib/ctypes/util.rs

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Applied to files:

• crates/vm/src/stdlib/ctypes/util.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/vm/src/stdlib/ctypes/pointer.rs

crates/vm/src/stdlib/ctypes/array.rs (1)

• create_array_type_with_stg_info (30-66)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

crates/vm/src/stdlib/ctypes/function.rs (3)

• obj (457-457)
• t (86-86)
• t (360-360)

crates/vm/src/stdlib/ctypes/field.rs (4)

• obj (107-107)
• obj (116-116)
• obj (201-201)
• obj (218-218)

crates/vm/src/stdlib/ctypes/array.rs (2)

• t (81-81)
• t (89-89)

crates/vm/src/stdlib/ctypes.rs (3)

• std (418-418)
• std (444-444)
• std (696-696)

crates/vm/src/stdlib/ctypes/array.rs (1)

• create_array_type_with_stg_info (30-66)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

crates/vm/src/stdlib/ctypes/util.rs (1)

• new_array (70-87)

• 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 (ubuntu-latest)
• GitHub Check: Check Rust code with clippy
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run tests under miri
• GitHub Check: Check the WASM package and demo
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run snippets and cpython tests on wasm-wasi

crates/vm/src/stdlib/ctypes/util.rs (1)

55-87: LGTM!

The new() and new_array() constructors are well-structured. The new_array() constructor properly initializes all array-specific fields including element_type and element_size, which aligns with the new type-data driven approach for ctypes metadata.

crates/vm/src/stdlib/ctypes/pointer.rs (2)

12-13: LGTM!

The PyCPointerType is now an empty struct with derived traits, consistent with the PR's pattern of moving type metadata to type_data storage. This simplifies the type while centralizing metadata management.

---

18-34: LGTM!

The __mul__ implementation correctly creates array types using the new factory-based approach. The pointer element size is correctly calculated as std::mem::size_of::<usize>(), and the StgInfo::new_array call properly captures all necessary metadata.

crates/vm/src/stdlib/ctypes/base.rs (3)

231-232: LGTM!

The PyCSimpleType is now an empty struct, consistent with the PR's pattern of deriving metadata through get_stg_info() rather than storing it directly.

---

237-249: LGTM!

The get_stg_info method correctly derives StgInfo from the _type_ attribute, validating that it's a single-character type code before computing size and alignment. The fallback to StgInfo::default() is appropriate for error cases.

---

743-774: LGTM!

The repeat method correctly creates array types using the new factory-based approach. The element size calculation properly handles simple types via their _type_ attribute with a sensible fallback to pointer size.

crates/vm/src/stdlib/ctypes/union.rs (1)

16-17: LGTM!

The PyCUnionType is now an empty struct with derived traits, consistent with the PR's pattern of moving type metadata to type_data storage. The derived Default is appropriate for the empty struct.

crates/vm/src/stdlib/ctypes.rs (2)

394-408: LGTM!

The size_of function now correctly prioritizes TypeDataSlot for retrieving size information. The two-step check (first on the instance's class, then on the type object itself) properly handles both instance and type queries, aligning with the new data-driven approach.

---

670-681: LGTM!

The alignment function follows the same pattern as size_of, correctly prioritizing TypeDataSlot for alignment information while maintaining backward compatibility with the fallback logic for types that don't use the new type_data mechanism.

crates/vm/src/stdlib/ctypes/structure.rs (1)

18-19: LGTM! Struct migration to type_data approach is clean.

The migration of PyCStructType to an empty struct with state stored in type_data aligns with the PR's broader refactoring goal and follows the same pattern used in PyCArrayType.

crates/vm/src/stdlib/ctypes/array.rs (3)

29-66: LGTM! Array type factory is well-structured.

The create_array_type_with_stg_info function properly creates dynamic array types with StgInfo stored in type_data. Error handling for the downcast is appropriate, and the fallback logic for already-initialized type_data is a good defensive measure.

---

26-27: LGTM! Type-data accessor pattern is appropriate.

The migration to an empty PyCArrayType struct with accessors reading from type_data is clean. The fallback patterns (unwrap_or_else(|| vm.ctx.none()) and unwrap_or(0)) provide reasonable defaults when type_data is missing.

Also applies to: 79-92

---

214-230: LGTM! AsNumber implementation delegates correctly.

The multiply implementation properly handles overflow checking and delegates to __mul__. The error handling is appropriate.