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

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

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

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.

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.

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

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

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.

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.

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