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

85-109: Consider extracting shared size calculation logic.

This get_field_size logic duplicates the element size derivation in base.rs (lines 311-324) and array.rs. Consider extracting this into a shared helper function in _ctypes to reduce duplication and ensure consistent behavior.

Example shared helper:

// In _ctypes module
pub fn get_field_size_from_type(field_type: &PyObjectRef, vm: &VirtualMachine) -> PyResult<usize> {
    // ... shared implementation
}

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

374-382: Silent failure when value is not bytes.

The set_value setter silently ignores non-PyBytes values and returns Ok(()). This could mask user errors. Consider raising a TypeError for consistency with set_raw.

     #[pygetset(setter)]
     fn set_value(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
         if let Some(bytes) = value.downcast_ref::<PyBytes>() {
             let mut buffer = self.buffer.write();
             let src = bytes.as_bytes();
             let len = std::cmp::min(src.len(), buffer.len());
             buffer[..len].copy_from_slice(&src[..len]);
+            Ok(())
+        } else {
+            Err(vm.new_type_error("expected bytes".to_owned()))
         }
-        Ok(())
     }

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

233-242: Inconsistent error handling: silently skipping malformed fields.

process_fields returns errors for malformed field tuples, but py_new silently skips them with continue. This inconsistency could cause hard-to-debug issues where a structure appears to work but has missing fields.

Consider reusing validation logic or returning errors consistently:

-            let Some(field_tuple) = field.downcast_ref::<PyTuple>() else {
-                continue;
-            };
-            if field_tuple.len() < 2 {
-                continue;
-            }
-            let Some(name) = field_tuple.first().unwrap().downcast_ref::<PyStr>() else {
-                continue;
-            };
+            let field_tuple = field
+                .downcast_ref::<PyTuple>()
+                .ok_or_else(|| vm.new_type_error("_fields_ must contain tuples".to_string()))?;
+            if field_tuple.len() < 2 {
+                return Err(vm.new_type_error("_fields_ tuple must have at least 2 elements".to_string()));
+            }
+            let name = field_tuple.first().unwrap()
+                .downcast_ref::<PyStr>()
+                .ok_or_else(|| vm.new_type_error("field name must be a string".to_string()))?;

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-25T11:05:02.899Z
Learning: Applies to **/src/**/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

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.

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

258-277: Potential panic if buffer slice is too short.

The bytes_to_int function directly indexes bytes[0], bytes[1], etc. without verifying slice length. If called with an undersized slice, this will panic.

Add a length guard at the start:

     fn bytes_to_int(bytes: &[u8], size: usize, vm: &VirtualMachine) -> PyObjectRef {
+        if bytes.len() < size {
+            return vm.ctx.new_int(0).into();
+        }
         match size {
             1 => vm.ctx.new_int(bytes[0] as i8).into(),

---

404-412: Lifetime concern: Arg::new(&ptr) references a stack-local pointer.

The ptr variable is local to this function. While Arg::new(&ptr) takes a reference to the pointer value itself (not the buffer data), this still creates an Arg that references memory that will be invalid after the function returns and the read guard is dropped.

This requires architectural changes to ensure the buffer outlives the FFI call. Consider storing the buffer pointer in a way that persists for the duration of the FFI call, or restructuring how arguments are passed to FFI.

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

106-134: Potential panic with unrecognized single-character type codes.

The get_size() function uses unreachable!() for unknown type strings. While the s.len() == 1 check filters multi-char strings, an unrecognized single character (e.g., "x") will still trigger a panic.

Validate the type code before calling get_size:

         .and_then(|type_str| {
             let s = type_str.to_string();
-            (s.len() == 1).then(|| get_size(&s))
+            const VALID_TYPES: &str = "cbBhHiIlqLQfdg?PzZOu";
+            (s.len() == 1 && VALID_TYPES.contains(&s)).then(|| get_size(&s))
         })

---

136-155: Element size is hardcoded to pointer size instead of actual structure size.

The element_size should be calculated from the structure's fields, not hardcoded to size_of::<usize>(). This will cause incorrect array sizing when creating arrays of structures.

Consider computing the actual structure size from _fields_:

     fn __mul__(cls: PyTypeRef, n: isize, vm: &VirtualMachine) -> PyResult {
         use super::array::{PyCArray, PyCArrayType};
         if n < 0 {
             return Err(vm.new_value_error(format!("Array length must be >= 0, not {n}")));
         }
-        // TODO: Calculate element size properly
-        // For structures, element size is the structure size (sum of field sizes)
-        let element_size = std::mem::size_of::<usize>(); // Default, should calculate from fields
+        // Calculate element size from _fields_
+        let element_size = if let Ok(fields_attr) = cls.as_object().get_attr("_fields_", vm) {
+            Self::calculate_struct_size_from_fields(&fields_attr, vm).unwrap_or(std::mem::size_of::<usize>())
+        } else {
+            std::mem::size_of::<usize>()
+        };

You'll need to add a helper method or reuse logic from process_fields to sum field sizes.

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

308-312: _fields_ getter returns None instead of actual fields.

The _fields_ property always returns vm.ctx.none() instead of retrieving the actual _fields_ from the class. While the comment suggests this is intentional, this may break code that expects to read _fields_ from an instance.

Consider returning the class's _fields_ attribute:

     #[pygetset]
     fn _fields_(&self, vm: &VirtualMachine) -> PyObjectRef {
-        // Return the _fields_ from the class, not instance
-        vm.ctx.none()
+        // Return the _fields_ from the class
+        self.class()
+            .get_attr(vm.ctx.intern_str("_fields_"), vm)
+            .unwrap_or_else(|_| vm.ctx.none())
     }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.899Z
Learning: Applies to **/src/**/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

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.

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

422-434: Windows todo!() will panic at runtime.

This was flagged in a previous review. The todo!() macro will cause a runtime panic on Windows. Consider returning 0 with a TODO comment, or raising a proper Python exception.

         #[cfg(target_os = "windows")]
         {
-            todo!("On Windows, use wcslen from ucrt")
+            // TODO: RUSTPYTHON - On Windows, should use wcslen from ucrt
+            // Returning 0 as placeholder until Windows support is added
+            0
         }

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

136-157: Type conversion only handles signed integers.

This was flagged in a previous review. The implementation always interprets bytes as signed integers, but ctypes supports unsigned types (c_uint, c_ulong) and floating-point types (c_float, c_double). Consider using the field's proto type information to determine the correct interpretation.

---

159-186: Silent truncation on integer overflow.

This was flagged in a previous review. Using unwrap_or(0) silently handles values that don't fit, whereas CPython ctypes raises OverflowError. For example, assigning 300 to a 1-byte field silently becomes 0.

             1 => {
-                let val = i.to_i8().unwrap_or(0);
+                let val = i.to_i8().ok_or_else(|| {
+                    vm.new_overflow_error("int too large to convert".to_owned())
+                })?;
                 Ok(val.to_ne_bytes().to_vec())
             }

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

258-277: Potential panic if buffer is too short.

This was flagged in a previous review. The bytes_to_int function accesses array indices without validating slice length. If called with an undersized slice, this will panic.

     fn bytes_to_int(bytes: &[u8], size: usize, vm: &VirtualMachine) -> PyObjectRef {
+        if bytes.len() < size {
+            return vm.ctx.new_int(0).into();
+        }
         match size {
             1 => vm.ctx.new_int(bytes[0] as i8).into(),
             2 => {
-                let val = i16::from_ne_bytes([bytes[0], bytes[1]]);
+                let val = i16::from_ne_bytes(bytes[..2].try_into().unwrap());
                 vm.ctx.new_int(val).into()
             }

---

373-382: set_value silently ignores non-bytes input.

This was flagged in a previous review. When value is not PyBytes, the function returns Ok(()) without any indication that the value wasn't set.

     fn set_value(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
         if let Some(bytes) = value.downcast_ref::<PyBytes>() {
             let mut buffer = self.buffer.write();
             let src = bytes.as_bytes();
             let len = std::cmp::min(src.len(), buffer.len());
             buffer[..len].copy_from_slice(&src[..len]);
+            Ok(())
+        } else {
+            Err(vm.new_type_error("expected bytes".to_owned()))
         }
-        Ok(())
     }

---

404-412: Lifetime issue with to_arg - pointer may dangle after read lock is released.

This was flagged in a previous review. The buffer.read() guard is dropped at the end of the function, but ptr still references that memory. The Arg returned will point to potentially invalid memory when used.

The TODO comment acknowledges this, but this is a use-after-free that will cause undefined behavior. Consider:

1. Storing a persistent buffer pointer in PyCArray that outlives FFI calls
2. Using Arc<RwLock<Vec<u8>>> and keeping the guard alive during FFI
3. Copying the buffer to a location that's guaranteed to outlive the call

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

106-134: Guard get_size against unknown _type_ codes to avoid VM panics

get_field_size calls get_size(&s) for any single‑char _type_ value without validating the code. If _type_ is a single unknown character (e.g. user‑defined or corrupted), get_size’s internal unreachable!() will panic the VM.

Consider explicitly validating the code before calling get_size, or delegating to a non‑panicking helper in _ctypes:

-            .and_then(|type_str| {
-                let s = type_str.to_string();
-                (s.len() == 1).then(|| get_size(&s))
-            })
+            .and_then(|type_str| {
+                let s = type_str.to_string();
+                if s.len() != 1 {
+                    return None;
+                }
+                // Keep this set in sync with `_ctypes::get_size`.
+                const VALID_TYPES: &[&str] = &[
+                    "u", "c", "b", "h", "H", "i", "I", "l", "q", "L", "Q", "f", "d", "g", "?",
+                    "B", "P", "z", "Z", "O",
+                ];
+                if !VALID_TYPES.contains(&s.as_str()) {
+                    return None;
+                }
+                Some(get_size(&s))
+            })

This keeps get_field_size total‑failure‑free and avoids panics even if _type_ is malformed.

---

136-175: Structure * n uses pointer size instead of structure size for arrays

PyCStructType::__mul__ sets:

// TODO: Calculate element size properly
let element_size = std::mem::size_of::<usize>();

For arrays of structures (Point * 10), this will allocate n * sizeof(usize) bytes instead of n * sizeof(struct). Any field descriptor that uses struct offsets larger than sizeof(usize) risks indexing past the end of the array buffer and panicking.

You should compute the real structure size from _fields_, ideally reusing the same logic as in PyCStructure::py_new:

-        // TODO: Calculate element size properly
-        // For structures, element size is the structure size (sum of field sizes)
-        let element_size = std::mem::size_of::<usize>(); // Default, should calculate from fields
+        // Element size is the structure size derived from the class's `_fields_`
+        let element_size = {
+            let fields_attr = cls
+                .as_object()
+                .get_attr("_fields_", vm)
+                .map_err(|_| vm.new_type_error("Structure has no _fields_".to_owned()))?;
+            // Factor this out of `PyCStructure::py_new` into a shared helper
+            PyCStructType::get_struct_size_from_fields(&fields_attr, vm)?
+        };

…and add a get_struct_size_from_fields helper that mirrors the size/offset accumulation in PyCStructure::py_new, so arrays and instances share one definition of layout.

Until this is fixed, arrays of non‑trivial Structure are very likely incorrect and fragile.

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

38-110: Code duplication with PyCStructType - consider extracting shared logic.

The process_fields and get_field_size methods are nearly identical to those in structure.rs (only differing in offset calculation - always 0 for unions vs. cumulative for structs). Consider extracting the common field parsing and size calculation into a shared helper module.

For example, a shared helper could be:

// In a common module
pub fn parse_fields(fields_attr: &PyObjectRef, vm: &VirtualMachine) 
    -> PyResult<Vec<(String, PyObjectRef, usize)>> { ... }

pub fn get_field_size(field_type: &PyObjectRef, vm: &VirtualMachine) 
    -> PyResult<usize> { ... }

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

248-256: int_to_bytes silently truncates on overflow (same issue as field.rs).

Using unwrap_or(0) silently handles values that don't fit. Consider returning a PyResult<Vec<u8>> and raising OverflowError for consistency with CPython.

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

306-313: Implement _fields_ getter for instances (currently always returns None)

The _fields_ getter on PyCStructure currently returns None, despite the comment saying it should expose the class’s _fields_. This breaks introspection like instance._fields_ and diverges from expectations around ctypes.Structure.

Now that you store fields: IndexMap<String, FieldInfo> and the class already has a _fields_ attribute, consider:

• Either returning the class attribute:

    #[pygetset]
    fn _fields_(&self, vm: &VirtualMachine) -> PyObjectRef {
-        // Return the _fields_ from the class, not instance
-        vm.ctx.none()
+        self.as_object()
+            .class()
+            .get_attr("_fields_", vm)
+            .unwrap_or_else(|_| vm.ctx.none())
    }

• Or reconstructing _fields_ from self.fields if you want the instance to be the single source of truth.

Either way, returning None permanently is likely to surprise users.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.899Z
Learning: Applies to Lib/**/*.py : When modifying files in Lib/, add a `# TODO: RUSTPYTHON` comment to mark modifications, or use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with a comment

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-25T11:05:02.899Z
Learning: Applies to **/src/**/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

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.