◐ Shell
clean mode source ↗

_ctypes pt. 5 by arihant2math · Pull Request #5653 · RustPython/RustPython

arihant2math

23-60: LGTM on ArgumentType trait design.

The trait-based approach for FFI type conversion is well-structured and extensible. The error handling properly uses PyResult throughout.

121-176: LGTM on constructor logic.

The constructor properly resolves library handles, loads symbols, and initializes the struct. Error handling uses map_err appropriately.

76-83: I'll verify the review comment about the incomplete from_ffi_type implementation. Let me start by exploring the context of this code.
<function_calls>

cat -n crates/vm/src/stdlib/ctypes/function.rs | head -100

</function_calls>

Let me get the file size first to understand its scope:
<function_calls>

wc -l crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Now let me read the relevant section:
<function_calls>

cat -n crates/vm/src/stdlib/ctypes/function.rs | sed -n '60,100p'

</function_calls>

Let me search for the struct/trait context and callers of this method:
<function_calls>

rg -n "from_ffi_type" crates/vm/src/stdlib/ctypes/ -B 3 -A 3

</function_calls>

Let me search for where from_ffi_type is actually invoked:
<function_calls>

rg -n ".from_ffi_type(" crates/vm/src/stdlib/ctypes/ -B 2 -A 2

</function_calls>

Let me look for the trait definition and understand the context better:
<function_calls>

cat -n crates/vm/src/stdlib/ctypes/function.rs | sed -n '1,85p'

</function_calls>

Let me search more broadly for usage patterns:
<function_calls>

rg -n "from_ffi_type" crates/vm/src/ --type=rs -B 2 -A 2

</function_calls>

Let me check what the trait definition looks like and if there are other implementations:
<function_calls>

rg -n "trait.*FFIType|impl.*FFIType" crates/vm/src/ --type=rs -B 1 -A 5

</function_calls>

Let me search for the FFIType trait definition:
<function_calls>

rg -n "trait.*Type" crates/vm/src/stdlib/ctypes/ --type=rs -B 1 -A 10 | head -80

</function_calls>

Let me get a broader view of the function.rs file:
<function_calls>

head -150 crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Now let me search for where the CDataType trait methods are called:
<function_calls>

rg -n "from_ffi_type" crates/ --type=rs | head -20

</function_calls>

<function_calls>

rg -n ".from_ffi_type" crates/ --type=rs

</function_calls>

Let me check if there are any tests for this functionality:
<function_calls>

find crates/ -name "test" -type f | grep -i ctypes

</function_calls>

<function_calls>

rg -n "todo!" crates/vm/src/stdlib/ctypes/ --type=rs

</function_calls>

Let me look at the surrounding context more carefully and check what trait this implements:
<function_calls>

sed -n '50,90p' crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Let me search for the CDataType trait definition:
<function_calls>

rg -n "trait CDataType" crates/vm/src/stdlib/ctypes/ -A 20

</function_calls>

Let me look at the entire ctypes module structure:
<function_calls>

ls -la crates/vm/src/stdlib/ctypes/

</function_calls>

<function_calls>

rg -n "impl.*CDataType" crates/vm/src/stdlib/ctypes/ --type=rs

</function_calls>

Let me check the mod.rs file:
<function_calls>

cat crates/vm/src/stdlib/ctypes/mod.rs | head -100

</function_calls>

Let me look at the types.rs to understand CDataType better:
<function_calls>

rg -n "trait CDataType|from_ffi_type" crates/vm/src/stdlib/ctypes/types.rs -B 2 -A 8 | head -80

</function_calls>

Now let me look at where from_ffi_type is actually called in the context of function calls:
<function_calls>

rg -n "from_ffi_type|to_ffi_value" crates/vm/src/stdlib/ctypes/ --type=rs -B 1 -A 1 | head -100

</function_calls>

Let me check the ffi.rs file to see if there's logic that calls from_ffi_type:
<function_calls>

ls -la crates/vm/src/stdlib/ctypes/ | grep -E ".rs$"

</function_calls>

<function_calls>

wc -l crates/vm/src/stdlib/ctypes/*.rs

</function_calls>

Let me check the module for function calls or invocations of from_ffi_type:
<function_calls>

rg -n "call.*function|invoke|CFuncPointer" crates/vm/src/stdlib/ctypes/mod.rs --type=rs -B 2 -A 5 | head -100

</function_calls>

Let me search for how CFunction is used:
<function_calls>

grep -n "CFunction|CFunc" crates/vm/src/stdlib/ctypes/function.rs | head -20

</function_calls>

Let me get the full context of the function.rs file around the CFunction struct:
<function_calls>

head -200 crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Let me search for where CFunction methods are called or where this gets instantiated:
<function_calls>

rg -n "CFunction|CFuncPointer" crates/vm/src/stdlib/ctypes/ --type=rs -B 1 -A 1 | grep -v "test" | head -60

</function_calls>

Let me check if there are any integration tests that use ctypes functions:
<function_calls>

find crates -name ".rs" -path "/test*" -o -name "test.rs" | xargs grep -l "ctypes" 2>/dev/null | head -5

</function_calls>

<function_calls>

rg -n "CFuncPointer" crates/ --type=rs | head -10

</function_calls>

Let me check if this is behind a feature gate or condition:
<function_calls>

sed -n '1,200p' crates/vm/src/stdlib/ctypes/function.rs | grep -E "#[|#cfg|#!|pub fn|pub struct|impl"

</function_calls>

Let me read the full function.rs file to understand context:
<function_calls>

cat crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Perfect! Now I have the full context. Let me verify if CFuncPointer is actually used by looking for instantiations and calls:
<function_calls>

rg -n "CFuncPointer|CFunction" crates/vm/src/ --type=rs -B 1 -A 1

</function_calls>

Now let me check what happens when a CFuncPointer is created and used - trace through the module's public API:
<function_calls>

rg -n "pub.*CFuncPointer|pub fn.*c_function" crates/vm/src/stdlib/ctypes/ --type=rs -B 2 -A 10

</function_calls>

Let me check if there's code that shows how this gets called:
<function_calls>

rg -n ".call(|invoke|call_with|with_result" crates/vm/src/stdlib/ctypes/ --type=rs -B 3 -A 3 | head -100

</function_calls>

Let me look at the mod.rs file more carefully to see the exported types:
<function_calls>

cat crates/vm/src/stdlib/ctypes/mod.rs | head -300

</function_calls>

Let me search for how return types are handled in this module:
<function_calls>

rg -n "restype|return_type" crates/vm/src/stdlib/ctypes/ --type=rs -B 2 -A 2

</function_calls>

Let me check the git history to understand the intent - is this a placeholder or WIP code:
<function_calls>

git log --oneline crates/vm/src/stdlib/ctypes/function.rs | head -10

</function_calls>

<function_calls>

git log -p --follow -S "from_ffi_type" crates/vm/src/stdlib/ctypes/function.rs | head -150

</function_calls>

Let me check if there are docs or comments about this being incomplete:
<function_calls>

rg -n "TODO|FIXME|WIP|stub|unimplemented|not.*implement" crates/vm/src/stdlib/ctypes/ --type=rs

</function_calls>

Now let me verify if the CFuncPointer type is exposed publicly and can actually be called:
<function_calls>

rg -n "pub.*struct CFuncPointer|pub.*CFuncPointer" crates/vm/src/stdlib/ctypes/ --type=rs

</function_calls>

Based on my exploration, let me now determine if this is a critical issue by searching for any runtime invocations of this:
<function_calls>

python3 << 'EOF'

Check if CFuncPointer is used in the public API

import subprocess
import re

Search for CFuncPointer usage in the codebase

result = subprocess.run(['rg', '-n', 'CFuncPointer', 'crates/', '--type=rs'],
capture_output=True, text=True)

lines = result.stdout.strip().split('\n')
print(f"Total CFuncPointer references: {len(lines)}")
print("\nAll references:")
for line in lines[:30]:
print(line)
EOF

</function_calls>

Let me verify the actual invocation path by checking if from_ffi_type is called through CDataType:
<function_calls>

grep -n "impl.*CDataType" crates/vm/src/stdlib/ctypes/types.rs | head -5

</function_calls>

<function_calls>

sed -n '1,100p' crates/vm/src/stdlib/ctypes/types.rs

</function_calls>

Let me check if CFunction implements CDataType or similar:
<function_calls>

rg -n "impl.*for.*CFuncPointer|impl.*CFuncPointer" crates/vm/src/stdlib/ctypes/function.rs -B 3 -A 15

</function_calls>

Now let me understand the actual situation - let me look at line 76-83 once more and check what trait this implements:
<function_calls>

sed -n '65,90p' crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Now I see it! Line 75 shows "impl CDataType for CFuncPointer". Let me understand the invocation path:
<function_calls>

rg -n "CDataType" crates/vm/src/stdlib/ctypes/ --type=rs -B 1 -A 1 | head -150

</function_calls>

Let me check for actual invocations of from_ffi_type through the trait:
<function_calls>

rg -n "dyn.*CDataType|as_data_type|.from_ffi_type" crates/vm/src/stdlib/ctypes/ --type=rs -B 2 -A 2

</function_calls>

Let me check types.rs to see where from_ffi_type is called:
<function_calls>

wc -l crates/vm/src/stdlib/ctypes/types.rs && echo "---" && cat crates/vm/src/stdlib/ctypes/types.rs | grep -n "from_ffi_type" -B 5 -A 5

</function_calls>

Let me search across the entire codebase for where this method is called:
<function_calls>

rg -n "from_ffi_type(" crates/ --type=rs

</function_calls>

Let me verify this is indeed in a struct that's part of the public API:
<function_calls>

sed -n '1,85p' crates/vm/src/stdlib/ctypes/function.rs | tail -40

</function_calls>

Let me check if CFuncPointer is used in Python binding code:
<function_calls>

rg -n "CFuncPointer|c_func|CFUNCTYPE" crates/vm/src/stdlib/ctypes/mod.rs --type=rs -B 2 -A 3 | head -100

</function_calls>

Let me look at the full mod.rs file to see what's exported:
<function_calls>

wc -l crates/vm/src/stdlib/ctypes/mod.rs && tail -300 crates/vm/src/stdlib/ctypes/mod.rs

</function_calls>

Let me check Python test files or documentation for ctypes function pointer usage:
<function_calls>

find . -name "*.py" -type f | xargs grep -l "CFUNCTYPE|c_func|function.*return" 2>/dev/null | head -5

</function_calls>

<function_calls>

find crates -name ".py" -o -name ".md" | xargs grep -i "function.*pointer|cfunctype|ctypes.*function" 2>/dev/null | head -20

</function_calls>

Let me verify if this is truly a critical runtime path by checking how to_ffi_value is used (the inverse operation):
<function_calls>

rg -n "to_ffi_value(" crates/ --type=rs

</function_calls>

Let me check if there's an actual implementation of the inverse method to understand the pattern:
<function_calls>

sed -n '65,95p' crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Let me see what to_ffi_value does for CFuncPointer:
<function_calls>

sed -n '60,100p' crates/vm/src/stdlib/ctypes/function.rs

</function_calls>

Let me check the exact line numbers again and get the full impl block:
<function_calls>

cat -n crates/vm/src/stdlib/ctypes/function.rs | sed -n '60,95p'

</function_calls>

Perfect. Now let me verify what the actual concern is - whether from_ffi_type returning None vs panicking matters:
<function_calls>

rg -n "impl.*CDataType" crates/vm/src/stdlib/ctypes/ --type=rs -A 20 | grep -E "from_ffi_type|to_ffi_value" -B 5 -A 5 | head -100

</function_calls>

Let me now verify the core claim: Is this actually a critical path for function call returns? Let me check a simple implementation:
<function_calls>

sed -n '1,50p' crates/vm/src/stdlib/ctypes/types.rs

</function_calls>

Let me look at what the return value means - checking a complete CDataType implementation:
<function_calls>

rg -n "from_ffi_type" crates/vm/src/stdlib/ctypes/types.rs -B 10 -A 15 | head -80

</function_calls>

Now let me check what Option means and if returning None is appropriate:
<function_calls>

rg -n "fn from_ffi_type" crates/vm/src/stdlib/ctypes/ --type=rs -A 8 | head -60

</function_calls>

Let me look at different implementations to understand the return type semantics:
<function_calls>

grep -n "fn from_ffi_type" crates/vm/src/stdlib/ctypes/types.rs

</function_calls>

<function_calls>

sed -n '240,260p' crates/vm/src/stdlib/ctypes/types.rs

</function_calls>

Let me search for the complete pattern of from_ffi_type implementations:
<function_calls>

python3 << 'EOF'
import subprocess
import re

Find all from_ffi_type implementations

result = subprocess.run(['rg', '-A', '5', 'fn from_ffi_type', 'crates/vm/src/stdlib/ctypes/', '--type=rs'],
capture_output=True, text=True)

print("from_ffi_type implementations:")
print(result.stdout[:2000])
EOF

</function_calls>

Let me check if there are tests that would reveal if this is actually called:
<function_calls>

find . -path "/test" -name "*.rs" | xargs grep -l "CFuncPointer|c_function|CFUNCTYPE" 2>/dev/null

</function_calls>

Let me check if there's a CI configuration that might show ctypes tests:
<function_calls>

find . -name ".yml" -o -name ".yaml" | xargs grep -l "ctypes" 2>/dev/null | head -3

</function_calls>

Let me summarize my findings in a verification script:
<function_calls>

python3 << 'EOF'
import subprocess

findings = []

Check 1: Is from_ffi_type called?

result = subprocess.run(['rg', 'from_ffi_type\(', 'crates/', '--type=rs'],
capture_output=True, text=True)
if result.stdout:
findings.append("✓ from_ffi_type IS called somewhere")
findings.append(f" Locations: {result.stdout[:200]}")
else:
findings.append("✗ from_ffi_type is NOT called anywhere in the codebase")

Check 2: Is CFuncPointer exported?

result = subprocess.run(['rg', 'pub.*CFuncPointer', 'crates/', '--type=rs'],
capture_output=True, text=True)
if result.stdout:
findings.append("✓ CFuncPointer is exported publicly")
findings.append(f" {result.stdout[:200]}")
else:
findings.append("✗ CFuncPointer is NOT publicly exported")

Check 3: Check line 81 specifically

result = subprocess.run(['sed', '-n', '76,83p', 'crates/vm/src/stdlib/ctypes/function.rs'],
capture_output=True, text=True)
findings.append(f"\nCode at lines 76-83:\n{result.stdout}")

for f in findings:
print(f)
EOF

</function_calls>

Based on my comprehensive verification, I can now provide the rewritten review comment:

from_ffi_type implementation is incomplete but not currently invoked.

The todo!() on line 81 is problematic only if the method is called. However, my search of the codebase reveals that from_ffi_type is never invoked anywhere, and CFuncPointer is not publicly exported from the ctypes module. This appears to be a stub implementation for a feature (function pointers as return types from C functions) that is not yet implemented or used.

If function pointer return types are intended to be supported in the future, the suggested fix is reasonable:

fn from_ffi_type(
    &self,
    _value: *mut ffi::c_void,
-    _vm: &VirtualMachine,
+    vm: &VirtualMachine,
) -> PyResult<Option<PyObjectRef>> {
-    todo!()
+    Err(vm.new_not_implemented_error("Return type conversion not yet implemented".to_string()))
}

However, if this feature is not planned, the stub can remain as-is or be removed entirely.

257-259: Based on my verification through web searches, I can now provide a rewritten review comment. The searches confirm that:

  1. Function pointers ARE Send + Sync by default in Rust, but FFI wrappers require careful thread-safety audits
  2. The TODO comment is a red flag indicating this was deliberately left as a known issue
  3. Best practices require either a proper SAFETY comment or synchronization guarantees
  4. Cannot definitively verify the specific implementation without direct code access

Replace the TODO: fix comment with a proper SAFETY justification or add synchronization.

The TODO: fix on these unsafe trait implementations indicates a known concern. While raw Rust function pointers (fn) are inherently Send + Sync, wrapping them in a #[pyclass] context requires verifying that:

  1. CodePtr (function pointer) is only read, never modified post-construction, and any referenced C code is thread-safe
  2. handler: PyObjectRef is either wrapped in PyRwLock<_> or protected by equivalent synchronization (PyO3 Py<T> is Send + Sync, but interior mutability requires guards)
  3. No GIL-only state is accessed by the wrapped function

If these hold, document the safety invariants in a SAFETY comment. If uncertain, wrap mutable or GIL-bound fields with Mutex or confine the type to single-threaded contexts using #[pyclass(unsendable)].