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

13-29: Silent error swallowing in calculate_struct_size.

Line 16 uses unwrap_or_default() which silently returns an empty vector if try_to_value fails. This could mask type errors in _fields_. Consider propagating the error or at least logging it.

🔎 Suggested improvement:

-        let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm).unwrap_or_default();
+        let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm)?;

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

11-28: Same silent error swallowing as in calculate_struct_size.

Line 14 uses unwrap_or_default() which silently returns an empty vector on error. Consider propagating the error for consistency with proper error handling.

---

358-413: SetAttr may call process_fields twice for _fields_ assignment.

When setting _fields_:

1. If a descriptor exists (lines 370-381), it calls the descriptor then process_fields at line 378
2. Otherwise, it stores the attribute (lines 385-402), then calls process_fields again at line 408

This means process_fields is called after every _fields_ assignment regardless of path, but in the descriptor path it's called twice (once at 378, and the function returns at 380 so actually only once there).

Wait, looking again: line 380 has return Ok(()) so the second call at 408 is not reached in the descriptor case. This is correct but the control flow is complex. Consider restructuring for clarity.

🔎 Suggested restructure for clarity:

 impl SetAttr for PyCUnionType {
     fn setattro(
         zelf: &Py<Self>,
         attr_name: &Py<PyStr>,
         value: PySetterValue,
         vm: &VirtualMachine,
     ) -> PyResult<()> {
         let pytype: &Py<PyType> = zelf.to_base();
         let attr_name_interned = vm.ctx.intern_str(attr_name.as_str());
+        let is_fields = attr_name.as_str() == "_fields_";
 
         // 1. First, do PyType's setattro (PyType_Type.tp_setattro first)
         // Check for data descriptor first
         if let Some(attr) = pytype.get_class_attr(attr_name_interned) {
             let descr_set = attr.class().mro_find_map(|cls| cls.slots.descr_set.load());
             if let Some(descriptor) = descr_set {
                 descriptor(&attr, pytype.to_owned().into(), value.clone(), vm)?;
-                // After successful setattro, check if _fields_ and call process_fields
-                if attr_name.as_str() == "_fields_"
-                    && let PySetterValue::Assign(fields_value) = value
-                {
+                if is_fields && let PySetterValue::Assign(fields_value) = value {
                     PyCUnionType::process_fields(pytype, fields_value, vm)?;
                 }
                 return Ok(());
             }
         }
 
         // Store in type's attributes dict
         match &value {
             PySetterValue::Assign(v) => {
                 pytype
                     .attributes
                     .write()
                     .insert(attr_name_interned, v.clone());
+                if is_fields {
+                    PyCUnionType::process_fields(pytype, v.clone(), vm)?;
+                }
             }
             PySetterValue::Delete => {
                 let prev = pytype.attributes.write().shift_remove(attr_name_interned);
                 if prev.is_none() {
                     return Err(vm.new_attribute_error(format!(
                         "type object '{}' has no attribute '{}'",
                         pytype.name(),
                         attr_name.as_str(),
                     )));
                 }
             }
         }
-
-        // 2. If _fields_, call process_fields (which checks FINAL internally)
-        if attr_name.as_str() == "_fields_"
-            && let PySetterValue::Assign(fields_value) = value
-        {
-            PyCUnionType::process_fields(pytype, fields_value, vm)?;
-        }
 
         Ok(())
     }
 }

crates/vm/src/buffer.rs (1)

264-281: The implicit fallthrough behavior for standalone T/X is correct but could be more explicit.

When T or X appears without a following {, the code falls through to FormatType::try_from(c), which returns an error since T and X are not valid FormatType variants. This is the intended behavior (standalone T/X are invalid), but the code would be clearer with an explicit check and error message explaining that these characters require the {} syntax when used.

Consider adding an explicit guard:

if (c == b'T' || c == b'X') && chars.peek() != Some(&b'{') {
    return Err("'T' and 'X' format specifiers require '{}'".to_owned());
}

This would make the constraint explicit rather than relying on FormatType::try_from to catch it implicitly.

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

943-963: is_cwchar_array_or_pointer panics if StgInfo is missing

is_cwchar_array_or_pointer uses .expect("array has StgInfo") / .expect("pointer has StgInfo"). If a user manages to construct a malformed PyCArray/PyCPointer subclass without proper StgInfo, this will panic the VM instead of raising a Python exception.

This is low‑probability but avoidable.

Suggested defensive change

-    if let Some(arr) = value.downcast_ref::<PyCArray>() {
-        let info = arr.class().stg_info_opt().expect("array has StgInfo");
-        let elem_type = info.element_type.as_ref().expect("array has element_type");
+    if let Some(arr) = value.downcast_ref::<PyCArray>() {
+        let info = if let Some(info) = arr.class().stg_info_opt() {
+            info
+        } else {
+            return false;
+        };
+        let elem_type = if let Some(elem_type) = info.element_type.as_ref() {
+            elem_type
+        } else {
+            return false;
+        };

Same pattern for the pointer branch.

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

871-910: Minor: slice ops ignore base buffer for arrays backed by external memory

getitem_by_slice and wchar_array_set_value always operate on zelf.0.buffer and ignore base / base_offset the way getitem_by_index does. For arrays created via from_buffer / views into other ctypes objects, slicing and the wchar setters will read/write the local buffer instead of the base buffer.

This is subtle and probably rare, but it makes slice semantics inconsistent with single‑index semantics on such views.

Direction for aligning slice ops with base-backed arrays

• Mirror the logic used in getitem_by_index:

  let base_obj = zelf.0.base.read().clone();
  let (buffer_lock, offset0) = if let Some(cdata) = base_obj
      .as_ref()
      .and_then(|b| b.downcast_ref::<super::PyCData>())
  {
      (&cdata.buffer, zelf.0.base_offset.load())
  } else {
      (&zelf.0.buffer, 0)
  };

• Then compute offsets relative to offset0 in both the c and u slice branches and in wchar setters.

Also applies to: 1141-1169

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

486-547: sizeof fallback behavior for non‑StgInfo ctypes types is a bit ad-hoc

The sizeof implementation tries several cases, then for simple types uses _type_ and get_size, but finally returns size_of::<usize>() or errors for many other cases. For some ctypes scenarios (custom simple subclasses, mis‑initialized types) this may yield surprising sizes instead of a clean error.

Not a blocker, but worth tightening once the rest of the ctypes semantics stabilize.

Possible cleanup direction

• Prefer consistently using StgInfo.size for any type that has it, including simple types, rather than recomputing from _type_.
• For types without StgInfo and without _type_/known metaclasses, consider always raising TypeError("this type has no size") instead of falling back to size_of::<usize>().

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

153-176: Known leak in Windows conv_param for str → wide pointer

The Windows branch of conv_param for PyStr intentionally allocates a Vec<u16>, leaks it with mem::forget, and returns its pointer:

let wide: Vec<u16> = s.as_str().encode_utf16().chain(std::iter::once(0)).collect();
let addr = wide.as_ptr() as usize;
std::mem::forget(wide);
return Ok((Type::pointer(), FfiArgValue::Pointer(addr)));

The comment notes this FIXME/leak. Given typical usage, this is probably acceptable for now (and mirrors CPython’s tendency to tie such allocations to _objects), but it is a real leak and will be hit for every Windows oledll string parameter converted this way.

Future improvement

Once the _objects / keep‑alive story for parameters is fully wired, this code should store the wide buffer in an owned object (e.g., a PyBytes/capsule referenced from the argument’s _objects) instead of leaking the Vec<u16>.

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

523-572: Negative pointer indices are stored in _objects under huge usize keys

setitem_by_index accepts index: isize (allowing negative indices, as C pointer arithmetic does), but when calling keep_ref it uses index as usize:

let offset = index * element_size as isize;
let addr = (ptr_value as isize + offset) as usize;
...
zelf.0.keep_ref(index as usize, value, vm)

For negative index, this wraps to a very large usize. Since _objects is only used to keep referents alive and not looked up by index later, this is mostly an internal bookkeeping quirk rather than a functional bug, but it’s confusing and may make debugging harder.

Possible clean‑up

• Either forbid negative indices in __setitem__ (matching typical ctypes usage) and guard before calling setitem_by_index.
• Or change keep_ref to accept a signed index (if feasible) so the stored key matches the logical index used in pointer arithmetic.

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 Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.

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.

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.

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

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: 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.

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

208-211: Apply .as_ref() explicitly for consistency.

In the multi-field case (line 215), there's an explicit .map(|value| value.as_ref()) to convert PyObjectRef to &PyObject. However, in the single-field case, value from zelf.first().unwrap() (which is &PyObjectRef) is passed directly to format_field. While implicit deref coercion should work, this creates an inconsistency that reduces code clarity.

🔎 Apply this diff for consistency:

         if field_names.len() == 1 {
-            let value = zelf.first().unwrap();
+            let value = zelf.first().unwrap().as_ref();
             let formatted = format_field((value, field_names[0]))?;
             (formatted, ",")

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

232-235: Clarify that get_align is only valid for simple types.

The comment states "for C types, alignment equals size" which isn't universally true (e.g., structs). Consider renaming or documenting that this only applies to the simple type codes handled by type_info().

🔎 Suggested documentation improvement:

-/// Get alignment for a simple type - for C types, alignment equals size
+/// Get alignment for a simple type code (b, B, h, H, i, I, etc.)
+/// For these primitive C types, natural alignment equals size.
 fn get_align(ty: &str) -> usize {
     get_size(ty)
 }

---

1216-1230: Dangerous COM interop—consider additional safeguards.

The vtable access and direct memory write are inherently unsafe. While this is necessary for COM interop, consider:

1. Validate vtable pointer: Ensure *iunknown (the vtable pointer) is non-null before dereferencing
2. Document safety requirements: The caller must ensure src_ptr is a valid IUnknown pointer

🔎 Add null check for vtable:

         if src_ptr != 0 {
             unsafe {
                 // IUnknown vtable: [QueryInterface, AddRef, Release, ...]
                 let iunknown = src_ptr as *mut *const usize;
                 let vtable = *iunknown;
+                if vtable.is_null() {
+                    return Ok(E_POINTER);
+                }
                 let addref_fn: extern "system" fn(*mut std::ffi::c_void) -> u32 =
                     std::mem::transmute(*vtable.add(1)); // AddRef is index 1
                 addref_fn(src_ptr as *mut std::ffi::c_void);
             }
         }

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

26-33: Minor: Unused return value from closed library.

When the library is closed (None), get_pointer returns a null pointer. This is used as a HashMap key in get_or_insert_lib. Consider adding a brief comment explaining that the key remains valid for identification purposes even after the library is closed.

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

69-74: Consider simplifying the type conversion.

The conversion from PyRef<Self> to PyTypeRef via PyObjectRef intermediate is verbose. Since PyCStructType is #[repr(transparent)] over PyType, you could potentially use upcast or to_base directly.

Alternative approach:

// Using the new to_base() method since PyCStructType wraps PyType
let new_type: &Py<PyType> = zelf.to_base();

Note: This would require adjusting subsequent code that expects ownership.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.

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

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.

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: 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: 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/union.rs (2)

553-577: Union buffer view now matches Structure’s 0‑dimensional semantics

PyCUnion::as_buffer now builds a BufferDescriptor with dim_desc: vec![] and itemsize = buffer_len, mirroring PyCStructure and CPython’s scalar ctypes behavior. This fixes the previous mismatch where unions appeared as 1‑D buffers.

Looks good.

---

12-29: Avoid silently treating malformed _fields_ as a zero-sized union

Both calculate_union_size and PyCUnion::init_pos_args use:

let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm).unwrap_or_default();

If _fields_ exists but isn’t a proper sequence (or try_to_value fails), these helpers quietly behave as if there were no fields at all, yielding size 0 and skipping positional initialization. That makes broken union definitions hard to debug.

As with the struct helper, prefer propagating the conversion error (or mapping it to a TypeError consistent with process_fields) when _fields_ is present, e.g.:

-    if let Ok(fields_attr) = cls.as_object().get_attr("_fields_", vm) {
-        let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm).unwrap_or_default();
+    if let Ok(fields_attr) = cls.as_object().get_attr("_fields_", vm) {
+        let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm)?;

and similarly in init_pos_args.

Also applies to: 487-515

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

369-385: Pointer restypes (c_char_p, c_wchar_p, c_void_p/POINTER) always return ints

ReturnType::from_ffi_type treats "P", "z", and "Z" uniformly:

Some("P") | Some("z") | Some("Z") => {
    vm.ctx.new_int(unsafe { *(value as *const usize) }).into()
}

And ctypes_callproc routes such restypes through RawResult::Pointer, which convert_raw_result also turns into a Python int.

That means:

• restype = c_char_p → Python int pointer, not bytes/None.
• restype = c_wchar_p → Python int pointer, not str/None.
• General pointer types (including c_void_p) are also exposed only as bare integers.

CPython’s ctypes decodes c_char_p and c_wchar_p restypes into Python bytes/str (or None for NULL), while pointer types may expose pointer objects or addresses.

You already have string/wstring logic (string_at_impl, wstring_at_impl) and array/pointer helpers; consider:

• Distinguishing "z" and "Z" in convert_raw_result / ReturnType::from_ffi_type, decoding the pointed-to string and returning bytes/str (or None), and
• Leaving "P"/general pointer types as the raw address or appropriate ctypes pointer object.

Right now all pointer restypes collapse to integers, which diverges notably from ctypes semantics.

Also applies to: 1376-1395

---

186-287: ArgumentType::convert_object still bypasses .from_param, limiting ctypes compatibility

The new convert_object for PyTypeRef adds better handling for pointers/structures and raw PyCSimple, but it still never consults the type’s from_param hook. Instead it:

• Tries downcast::<PyCSimple>(), or
• Calls the type as a constructor and expects a PyCSimple.

This misses the primary ctypes customization point (from_param) for both built-in and user-defined types, and was already a pain point in the previous version.

Consider:

• First attempting to call from_param on the type (or using a shared paramfunc) if it exists, and using that result as the canonical converted object.
• Only falling back to the current downcast/constructor path when from_param is absent or clearly unsupported.

Without that, many legitimate argtypes patterns (especially custom Structure/Union/pointer subclasses) will not behave like CPython.

---

723-766: COM-style constructor still panics on non-Windows via NULL function pointer

The COM-method constructor branch:

if args.args.len() >= 2
    && first_arg.try_int(vm).is_ok()
    && args.args[1].downcast_ref::<PyStr>().is_some()
{
    // ...
    return PyCFuncPtr { _base: PyCData::from_bytes(vec![0u8; ptr_size], None), ... };
}

runs on all platforms. On non-Windows it creates a PyCFuncPtr with a NULL function pointer and no index/iid. Later, Callable::call does:

let (func_ptr, is_com_method) = (None::<CodePtr>, false); // non-Windows
let code_ptr = func_ptr.unwrap_or_else(|| zelf.get_code_ptr().expect("Function pointer should be set"));

Since get_code_ptr() returns None for a NULL pointer, this .expect will panic at call time.

You should either:

• Gate this entire COM constructor branch under #[cfg(windows)], or
• On non-Windows, detect this usage and raise a TypeError immediately instead of constructing a broken object.

As-is, non-Windows builds can create CFuncPtr instances that inevitably panic when called.

Also applies to: 1462-1480

---

1398-1452: OUT parameters currently discard the function’s own return value

build_result handles OUT buffers like this:

if prepared.out_buffers.is_empty() {
    return result.map(Ok).unwrap_or_else(|| Ok(vm.ctx.none()));
}

let out_values = extract_out_values(prepared.out_buffers, vm);
Ok(match <[PyObjectRef; 1]>::try_from(out_values) {
    Ok([single]) => single,
    Err(v) => PyTuple::new_ref(v, &vm.ctx).into(),
})

As soon as any OUT parameters exist, the function’s (possibly errcheck-processed) return value is dropped and only the OUT values are returned. In CPython ctypes, the usual pattern is to return a tuple like (result, *outs).

You likely want:

• To keep the existing “no OUT” branch as-is, and
• When OUT buffers are present, return a tuple whose first element is result.unwrap_or(vm.ctx.none()) followed by the extracted OUT values.

That way, callers with both a meaningful return value and OUT/INOUT parameters see the full result, not just the OUTs.

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

13-29: Don’t silently drop _fields_ conversion errors in calculate_struct_size

calculate_struct_size uses:

let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm).unwrap_or_default();

If _fields_ is malformed or try_to_value fails, you quietly return size 0 instead of surfacing an error, which makes misconfigured structures hard to debug and can mask real bugs. Prefer propagating the error (or mapping it to a descriptive TypeError) when _fields_ exists.

A minimal fix:

-    if let Ok(fields_attr) = cls.as_object().get_attr("_fields_", vm) {
-        let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm).unwrap_or_default();
+    if let Ok(fields_attr) = cls.as_object().get_attr("_fields_", vm) {
+        let fields: Vec<PyObjectRef> = fields_attr.try_to_value(vm)?;

This keeps the Ok(0) fallback for the “no _fields_ attribute” case but no longer hides bad _fields_ contents.

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

64-73: Guard against element_size * length overflow in array_type_from_ctype

Here you compute the total array size as element_size * length without checking for overflow:

let stg_info = StgInfo::new_array(
    element_size * length,
    element_align,
    length,
    ...
);

For large length or element_size, this can overflow usize (panic in debug, wrap in release) and produce a bogus layout. In PyCArrayType::init you already use a length > usize::MAX / item_size guard; mirror that here before calling StgInfo::new_array.

Example adjustment:

-    let stg_info = StgInfo::new_array(
-        element_size * length,
+    if element_size != 0 && length > usize::MAX / element_size {
+        return Err(vm.new_overflow_error("array too large"));
+    }
+    let stg_info = StgInfo::new_array(
+        element_size * length,
         element_align,
         length,
         ...
     );

---

613-638: c_wchar_p reading still hard-codes 32‑bit wchar and ignores WCHAR_SIZE helpers

The "Z" branch in read_element_from_buffer still does:

let ptr = ptr_val as *const u32;
while *ptr.add(len) != 0 { ... }
let wchars = std::slice::from_raw_parts(ptr, len);
let s: String = wchars.iter().filter_map(|&c| char::from_u32(c)).collect();

This assumes 32‑bit wchar_t everywhere. On Windows, wchar_t is 16‑bit, and you already introduced WCHAR_SIZE, wchar_from_bytes, and wstring_from_bytes that correctly handle platform differences.

You should decode the pointed‑to buffer using those helpers instead of hard‑coding u32, e.g.:

• Treat ptr_val as *const u8.
• Walk in WCHAR_SIZE‑sized chunks until a 0 code unit.
• Use wchar_from_bytes/wstring_from_bytes to build the String.

This will make "Z" consistent with the array and helper APIs and avoid misaligned, incorrect reads on Windows.

Also applies to: 1220-1268

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

66-86: Library cache no longer actually reuses existing libraries

get_or_insert_lib computes key from the freshly created SharedLibrary’s Library pointer, then immediately looks up self.libraries.get(&key). That key has never been inserted before, so the Some(l) branch (with is_closed check) is effectively unreachable, and a new SharedLibrary is always inserted.

If you intend _ctypes to reuse an existing handle for the same library (or at least to make the “closed → replace” path meaningful), the key should be derived from something stable across calls (e.g., the OS handle or canonicalized path) rather than the address of the just-created Library.

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

449-471: Document memory safety assumptions for pointer slice operations

In getitem_by_slice for "c" (c_char) types, raw pointer slices are created without bounds validation:

if step == 1 {
    // Optimized contiguous copy
    let start_addr = (ptr_value as isize + start * element_size as isize) as *const u8;
    unsafe {
        result.extend_from_slice(std::slice::from_raw_parts(start_addr, len));
    }
}

There's no validation that:

• ptr_value points to valid, allocated memory
• The slice [start_addr, start_addr + len) stays within valid bounds
• The memory region has the expected lifetime

While pointer arithmetic in ctypes inherently requires callers to ensure validity (matching CPython's behavior), the lack of documentation makes it unclear whether this is intentional. Consider:

1. Adding a comment documenting that callers must ensure the pointer and computed offsets are valid
2. Optionally, checking ptr_value != 0 before slice operations (though CPython allows this for non-zero invalid pointers)
3. For development builds, consider optional bounds checking via a debug feature flag

Similar patterns appear at lines 481-490 (c_wchar slice) and throughout read/write operations.

Note: This is standard ctypes behavior—users working with raw pointers are responsible for memory safety—but explicit documentation improves maintainability.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.

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: 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.

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-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

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)

64-73: Guard against element_size * length overflow when constructing array StgInfo

StgInfo::new_array is called with element_size * length without overflow checking. For large length or element_size this can overflow usize, corrupting the computed size and later allocations (e.g. in PyCArray::slot_new’s buffer).

You already have a safe pattern in PyCArrayType::Initializer (length > usize::MAX / item_size): mirror that here (e.g. checked_mul + OverflowError) before calling StgInfo::new_array.

Suggested fix sketch

-    let stg_info = StgInfo::new_array(
-        element_size * length,
+    let total_size = element_size
+        .checked_mul(length)
+        .ok_or_else(|| vm.new_overflow_error("array too large"))?;
+    let stg_info = StgInfo::new_array(
+        total_size,
         element_align,
         length,
         itemtype_ref.clone(),
         element_size,
         item_format.as_deref(),
         &item_shape,
         item_flags,
     );

---

613-637: c_wchar_p reader hardcodes 32‑bit wchar and ignores WCHAR_SIZE

The "Z" arm of read_element_from_buffer interprets the target as *const u32, assuming 4‑byte wide chars. On Windows wchar_t is 2 bytes; on other platforms you already expose WCHAR_SIZE, wchar_from_bytes, and wstring_from_bytes for platform‑correct handling.

This mismatch will break c_wchar_p arrays on Windows (wrong alignment/size) and duplicates logic that should be centralized in the wchar helpers.

Refactor the "Z" arm to use WCHAR_SIZE + wstring_from_bytes (or a shared helper in base.rs) rather than *const u32, so wide pointer reading matches the rest of the ctypes wchar handling.

Suggested direction (high level)

• Keep reading the pointer value as you do now.
• Use std::slice::from_raw_parts(ptr as *const u8, len_in_bytes) and then wstring_from_bytes to build the String.
• Compute len_in_bytes as n * WCHAR_SIZE where n is the number of wchar elements up to the terminator, using wchar_from_bytes to walk until 0.

Also applies to: 1214-1262

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

1377-1395: Pointer restypes (c_char_p / c_wchar_p / c_void_p) are always exposed as Python ints

convert_raw_result maps RawResult::Pointer directly to vm.ctx.new_int(*ptr), and ReturnType::from_ffi_type also returns ints for type codes "P", "z", and "Z". Combined with CallInfo::is_pointer_return, this means:

• restype = c_char_p yields an integer address, not bytes/None.
• restype = c_wchar_p yields an integer address, not str/None.
• restype = c_void_p also yields a bare int, which is arguable but inconsistent with ctypes’ pointer object behavior.

This diverges from ctypes, where "z"/"Z" restypes decode the pointed‑to C string and return bytes/str (or None), while generic pointers expose addresses.

Suggested high-level fix

• In convert_raw_result, when RawResult::Pointer(ptr) and call_info.restype_obj is a type with _type_ "z" or "Z", read from foreign memory:
  • "z": if ptr == 0 return None, else read a char* C string (like string_at_impl) and return PyBytes.
  • "Z": similarly, read a wchar_t* string using the same platform‑aware logic as wstring_at_impl and return PyStr.
• For "P" and other pointer types, keep returning the integer address or wrap it in the appropriate ctypes object.
• Optionally, reuse string_at_impl/wstring_at_impl to avoid duplicating the memory‑walk logic.

```web How does CPython ctypes treat function results when restype is c_char_p or c_wchar_p? ```

---

1409-1452: OUT parameters currently discard the function’s own return value

build_result returns only OUT values when prepared.out_buffers is non‑empty:

if prepared.out_buffers.is_empty() {
    return result.map(Ok).unwrap_or_else(|| Ok(vm.ctx.none()));
}
let out_values = extract_out_values(prepared.out_buffers, vm);
Ok(match <[PyObjectRef; 1]>::try_from(out_values) { ... })

This drops the function’s result (even post‑errcheck), whereas ctypes normally returns (result, *outs) when paramflags specify OUT/INOUT parameters.

Suggested behavior

• Keep the current branch when out_buffers is empty.
• When there are OUT values, construct a tuple whose first element is result.unwrap_or(vm.ctx.none()), followed by the extracted out values.
• Preserve the single‑OUT special case only for compatibility if you explicitly want it, but align with ctypes’ (result, out1, out2, ...) semantics.

```web In CPython ctypes, what is returned when a function has both a non-void restype and OUT parameters via paramflags? ```

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

59-75: c_char_p / c_wchar_p still reject None in constructors and .value assignments

set_primitive for "z" and "Z" only accepts ints/bytes/str:

"z" => {
    if value.downcast_ref_if_exact::<PyInt>(vm).is_some()
        || value.downcast_ref_if_exact::<PyBytes>(vm).is_some()
    { ... } else { TypeError(...) }
}
"Z" => {
    if value.downcast_ref_if_exact::<PyStr>(vm).is_some() { ... } else { TypeError(...) }
}

PyCSimple::slot_new and set_value both route explicit arguments through set_primitive when they’re not the special bytes/str fast paths. As a result:

• c_char_p(None) and c_wchar_p(None) raise TypeError instead of representing a NULL pointer.
• x.value = None for these types also raises, rather than clearing the pointer.

Although from_param accepts None for "z", "Z", and "P", constructors and attribute setters remain incompatible with ctypes semantics.

Suggested fix

• Adjust set_primitive to accept None for "z" and "Z" (similar to "P"):

- "z" => {
-     if value.downcast_ref_if_exact::<PyInt>(vm).is_some()
-         || value.downcast_ref_if_exact::<PyBytes>(vm).is_some()
-     {
-         Ok(value.to_owned())
-     } else {
-         Err(vm.new_type_error(...))
-     }
- }
+ "z" => {
+     if value.downcast_ref_if_exact::<PyInt>(vm).is_some()
+         || value.downcast_ref_if_exact::<PyBytes>(vm).is_some()
+         || value.downcast_ref_if_exact::<PyNone>(vm).is_some()
+     {
+         Ok(value.to_owned())
+     } else {
+         Err(vm.new_type_error("bytes, integer address or None expected"))
+     }
+ }

• Do the analogous change for "Z" (accept PyNone and mention “or None” in the error).
• With that in place, slot_new and set_value will naturally allow None and encode it as a NULL pointer (all‑zero buffer) via value_to_bytes_endian.

Also applies to: 140-173, 961-1003, 1167-1221

---

705-713: c_wchar serialization ignores actual wchar_t size (hardcoded 4 bytes)

In value_to_bytes_endian, the "u" (c_wchar) branch always encodes as a 4‑byte u32 and returns vec![0; 4] on fallback, regardless of platform. Other parts of the ctypes code (e.g., the wchar helpers in array.rs / base.rs) correctly use size_of::<libc::wchar_t>() via WCHAR_SIZE.

On Windows, where wchar_t is 2 bytes, this will desynchronize simple c_wchar layout from arrays/pointers and from the actual ABI, risking data corruption.

Suggested adjustment

• Use the same platform‑dependent size as the rest of the ctypes code:

- "u" => {
-     // c_wchar - 4 bytes (wchar_t on most platforms)
-     if let Ok(s) = value.str(vm)
-         && let Some(c) = s.as_str().chars().next()
-     {
-         return to_bytes!(c as u32);
-     }
-     vec![0; 4]
- }
+ "u" => {
+     use crate::stdlib::ctypes::array::WCHAR_SIZE; // or shared base helper
+     if let Ok(s) = value.str(vm)
+         && let Some(c) = s.as_str().chars().next()
+     {
+         let mut buf = vec![0u8; WCHAR_SIZE];
+         super::array::wchar_to_bytes(c as u32, &mut buf);
+         return buf;
+     }
+     vec![0; WCHAR_SIZE]
+ }

• Or equivalently, reuse the shared WCHAR_SIZE / wchar_to_bytes from the common wchar helper module.

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

646-669: write_value_at_address "Z" branch still leaks wchar buffers and diverges from the safer make_wchar_buffer pattern

The "Z" arm of write_value_at_address constructs a Vec<libc::wchar_t>, takes its pointer, and mem::forgets it:

let mut wchars: Vec<libc::wchar_t> = s
    .as_str()
    .chars()
    .map(|c| c as libc::wchar_t)
    .chain(std::iter::once(0))
    .collect();
let ptr = wchars.as_mut_ptr();
std::mem::forget(wchars);
ptr as usize

This heap allocation is never tracked or freed. Meanwhile, setitem_by_index has its own "Z"+PyStr special case that uses make_wchar_buffer and stores a holder in buffer_holder, so you effectively have two divergent code paths for the same type:

• This helper leaks memory on every call.
• The higher-level setter uses a safer holder, but nothing prevents future callers from hitting this helper directly.

Suggested refactor

• Remove the ad‑hoc Vec<libc::wchar_t> + mem::forget logic and instead:

  • Reuse make_wchar_buffer (as in setitem_by_index) to build a managed buffer and return its pointer.
  • Ensure the corresponding owner object is kept alive via keep_ref or a dedicated holder, not just a single shared buffer_holder.

• If you want a single authoritative path, consider making setitem_by_index always call a small helper that:

  • Builds/keeps the holder.
  • Writes the pointer.
  • Registers the holder for lifetime tracking.

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

500-535: Apply consistent wide-string handling to callback "Z" type, matching wstring_at_impl platform logic

In ffi_to_python, the "Z" (c_wchar_p) handler at lines 1631-1645 naively casts each wchar_t to char without platform branching, causing UTF-16 surrogate pairs on Windows to be mishandled. Align with the correct platform-aware logic used in wstring_at_impl: use Wtf8Buf::from_wide on Windows and char::from_u32 on other platforms.

Similarly, in python_to_ffi at line 1702, "Z" is treated as a raw integer pointer rather than a true wchar buffer. For callback returns, create and manage a wchar buffer (similar to make_wchar_buffer pattern) and return its pointer, keeping the buffer alive during callback execution to match non-callback c_wchar_p semantics.

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: 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.