Add capsule support to c-api#7940
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PyCapsule support to the C-ABI: capsule objects now hold an optional name and a context pointer; Context::new_capsule accepts a name; FFI utilities for const C strings added; and a full set of PyCapsule C-ABI functions (New, Get/Set pointer/context/name, IsValid, Import) with tests are implemented. ChangesPyCapsule C-ABI Implementation with Name and Context Support
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
crates/capi/src/util.rs (1)
79-85: 💤 Low valueConsider using
null()for const pointer.
core::ptr::null()is more idiomatic for*consttypes thannull_mut(), even though both work due to coercion.♻️ Suggested change
impl FfiResult for *const c_char { - const ERR_VALUE: *const c_char = core::ptr::null_mut(); + const ERR_VALUE: *const c_char = core::ptr::null(); fn into_output(self, _vm: &VirtualMachine) -> *const c_char { self } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/capi/src/util.rs` around lines 79 - 85, The impl of FfiResult for *const c_char uses core::ptr::null_mut() for ERR_VALUE; change it to core::ptr::null() to be idiomatic for const pointers (update the const ERR_VALUE in the impl and leave into_output(self, _vm: &VirtualMachine) -> *const c_char unchanged) so the null value reflects a *const type.crates/capi/src/pycapsule.rs (1)
134-149: ⚖️ Poor tradeoffTest uses pyo3's
PyCapsuleinstead of testing the C-API functions directly.The test validates capsule behavior through pyo3's wrapper API (using
PyCapsule::new_with_value(),is_valid_checked(),pointer_checked()), but does not exercise the C-API functions defined in this file (PyCapsule_New,PyCapsule_GetPointer,PyCapsule_GetName, etc.). Consider adding tests that call the C-API functions directly to verify the implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/capi/src/pycapsule.rs` around lines 134 - 149, The test currently exercises pyo3's PyCapsule wrapper but not the raw C-API implementations; add unit tests that directly call the C-API functions (e.g., PyCapsule_New, PyCapsule_GetPointer, PyCapsule_GetName, PyCapsule_IsValid) to validate your bindings: create a capsule via PyCapsule_New with a known pointer and name, assert PyCapsule_GetName returns the expected name, assert PyCapsule_GetPointer returns the original pointer (and that casting yields the original value), and test PyCapsule_IsValid for matching and non-matching names; place these tests alongside the existing ones and use Python::attach to obtain a PyO3 GIL for calling the unsafe C-API functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/pycapsule.rs`:
- Around line 41-49: The function PyCapsule_GetContext dereferences the raw
pointer `capsule` without checking for null; update it to first check if
`capsule` is null and return a null pointer on error, then proceed to
dereference and call downcast_ref_if_exact::<PyCapsule>(vm) as before (preserve
the existing with_vm usage and error path). Ensure the function returns a *mut
c_void null when the input is null or when downcast/with_vm fails, and keep
references to symbols PyCapsule_GetContext, with_vm, and
downcast_ref_if_exact::<PyCapsule> to locate the change.
- Around line 51-59: In PyCapsule_SetContext add a null-pointer check for the
incoming capsule before performing unsafe dereference: if capsule.is_null()
return an error (e.g., propagate a failure c_int) and set a Python error via the
VM (use vm.new_value_error or similar) rather than dereferencing; then proceed
to call downcast_ref_if_exact::<PyCapsule> and capsule.set_context(context) only
after the pointer is confirmed non-null.
- Around line 31-39: PyCapsule_GetName dereferences the raw pointer without a
null check which can UB if caller passes null; modify PyCapsule_GetName to
early-return a null C string pointer when capsule.is_null() (mirroring
PyCapsule_GetPointer), i.e., check the raw capsule pointer before the unsafe {
&*capsule } dereference and return std::ptr::null() (or equivalent) on null;
keep the rest of the with_vm flow and the downcast_ref_if_exact::<PyCapsule>(vm)
handling unchanged so valid capsules still resolve to
capsule.name().map(CStr::as_ptr).unwrap_or_default().
- Around line 26-29: The function PyCapsule_GetPointer dereferences capsule
without checking for null; add a null check at the start (if capsule.is_null())
and return std::ptr::null_mut() immediately to avoid undefined behavior, then
call with_vm and checked_capsule using unsafe { &*capsule } only after the null
check; mirror the null-handling pattern used in PyCapsule_IsValid to keep
behavior consistent.
- Around line 82-102: The function PyCapsule_Import must check for a null name
pointer before calling CStr::from_ptr to avoid UB; in PyCapsule_Import add an
initial null check (if name.is_null()) that sets an appropriate Python error via
the VM (e.g., vm.new_system_error or vm.new_import_error) and returns a null
*mut c_void, then proceed with the existing logic that calls CStr::from_ptr,
vm.import, attribute traversal, and checked_capsule; ensure the null-check is
performed before any unsafe dereference and that the function consistently
returns a null pointer when an error is set.
- Around line 61-69: The PyCapsule_SetPointer implementation must validate the
incoming raw pointers before dereferencing: check that the `capsule` (raw *mut
PyObject) is not null and return an error c_int if it is, then inside with_vm
still downcast the referenced object to `PyCapsule` as currently done;
additionally, enforce that the `pointer` argument is not null and return the
CPython-compatible error code when it is (matching other capsule APIs in this
crate). Update the function handling around `PyCapsule_SetPointer`, referencing
the `PyCapsule` type, the `with_vm` wrapper, and the `capsule`/`pointer`
parameters so null checks occur before any unsafe dereference and the function
returns the appropriate c_int error on null inputs.
In `@crates/vm/src/builtins/capsule.rs`:
- Line 17: The field and accessor for the capsule name currently use
Option<&'static CStr> which is too strict; change the capsule struct field
(currently "name: Option<&'static CStr>") to store a raw pointer (Option<*const
c_char>) and adjust the getter (the method returning the name at lines ~30–33
and similar at ~61–63) to return Option<&CStr> with a lifetime tied to &self by
checking for null and performing an unsafe dereference (e.g., convert the
pointer to &CStr inside the getter), ensuring you only assume the pointer is
valid for the capsule's lifetime rather than 'static; update all places
referencing the original &'static CStr type accordingly.
---
Nitpick comments:
In `@crates/capi/src/pycapsule.rs`:
- Around line 134-149: The test currently exercises pyo3's PyCapsule wrapper but
not the raw C-API implementations; add unit tests that directly call the C-API
functions (e.g., PyCapsule_New, PyCapsule_GetPointer, PyCapsule_GetName,
PyCapsule_IsValid) to validate your bindings: create a capsule via PyCapsule_New
with a known pointer and name, assert PyCapsule_GetName returns the expected
name, assert PyCapsule_GetPointer returns the original pointer (and that casting
yields the original value), and test PyCapsule_IsValid for matching and
non-matching names; place these tests alongside the existing ones and use
Python::attach to obtain a PyO3 GIL for calling the unsafe C-API functions.
In `@crates/capi/src/util.rs`:
- Around line 79-85: The impl of FfiResult for *const c_char uses
core::ptr::null_mut() for ERR_VALUE; change it to core::ptr::null() to be
idiomatic for const pointers (update the const ERR_VALUE in the impl and leave
into_output(self, _vm: &VirtualMachine) -> *const c_char unchanged) so the null
value reflects a *const type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 73238666-03e7-42c7-8749-db5d7d9f8378
📒 Files selected for processing (5)
crates/capi/src/lib.rscrates/capi/src/pycapsule.rscrates/capi/src/util.rscrates/vm/src/builtins/capsule.rscrates/vm/src/vm/context.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/capi/src/pycapsule.rs (1)
139-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest does not exercise the C-ABI functions defined in this module.
The test uses pyo3's
PyCapsule::new_with_valueand pyo3 methods (is_valid_checked,pointer_checked), not thePyCapsule_New,PyCapsule_IsValid, orPyCapsule_GetPointerfunctions implemented above. This leaves the actual C-ABI implementation untested.Consider calling the exported functions directly to validate the implementation:
#[test] fn test_capsule_new() { // Initialize RustPython VM and call PyCapsule_New, PyCapsule_IsValid, PyCapsule_GetPointer // to test the actual C-ABI surface }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/capi/src/pycapsule.rs` around lines 139 - 154, The test currently uses pyo3 helpers instead of the module's C-ABI exports; update test_capsule_new to call the exported functions directly: call PyCapsule_New to create the capsule, then call PyCapsule_IsValid and PyCapsule_GetPointer to validate and extract the pointer, cast the returned void* to the original String type and assert the value matches; ensure you call these functions through the crate's exported symbols (PyCapsule_New, PyCapsule_IsValid, PyCapsule_GetPointer) inside the Python::with_gil / Python::attach context and handle unsafe pointer casts properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/capi/src/pycapsule.rs`:
- Around line 139-154: The test currently uses pyo3 helpers instead of the
module's C-ABI exports; update test_capsule_new to call the exported functions
directly: call PyCapsule_New to create the capsule, then call PyCapsule_IsValid
and PyCapsule_GetPointer to validate and extract the pointer, cast the returned
void* to the original String type and assert the value matches; ensure you call
these functions through the crate's exported symbols (PyCapsule_New,
PyCapsule_IsValid, PyCapsule_GetPointer) inside the Python::with_gil /
Python::attach context and handle unsafe pointer casts properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 94984234-5ce2-47ec-bc1d-9a78e3b61558
📒 Files selected for processing (1)
crates/capi/src/pycapsule.rs
bbafa57 to
6a9f1a6
Compare
6a9f1a6 to
a34acf0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/capi/src/pycapsule.rs (1)
145-160: ⚡ Quick winTest exercises pyo3's
PyCapsule, not the RustPython C-ABI functions.This test creates and validates a capsule using
pyo3::types::PyCapsule, but it doesn't call any of thePyCapsule_New,PyCapsule_GetPointer,PyCapsule_IsValid, etc. functions defined in this file. Consider adding tests that invoke the actual exported C-ABI functions to verify the RustPython implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/capi/src/pycapsule.rs` around lines 145 - 160, The current test only uses pyo3's PyCapsule wrapper; add a new test (e.g., test_capsule_c_abi) that exercises the exported C-ABI functions: call PyCapsule_New to create a capsule with a C name, then call PyCapsule_IsValid (or PyCapsule_IsValid) to check the name, call PyCapsule_GetPointer to retrieve the pointer and verify the stored value, and finally call PyCapsule_SetDestructor or PyCapsule_SetPointer/appropriate cleanup if needed; use pyo3::ffi (ffi::PyCapsule_New, ffi::PyCapsule_IsValid, ffi::PyCapsule_GetPointer) with CString for the name, acquire the GIL via Python::with_gil/attach, check for null returns and use unsafe casts to compare the pointed-to Rust value, and add assertions mirroring the existing test to validate the RustPython C-ABI implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/capi/src/pycapsule.rs`:
- Around line 145-160: The current test only uses pyo3's PyCapsule wrapper; add
a new test (e.g., test_capsule_c_abi) that exercises the exported C-ABI
functions: call PyCapsule_New to create a capsule with a C name, then call
PyCapsule_IsValid (or PyCapsule_IsValid) to check the name, call
PyCapsule_GetPointer to retrieve the pointer and verify the stored value, and
finally call PyCapsule_SetDestructor or PyCapsule_SetPointer/appropriate cleanup
if needed; use pyo3::ffi (ffi::PyCapsule_New, ffi::PyCapsule_IsValid,
ffi::PyCapsule_GetPointer) with CString for the name, acquire the GIL via
Python::with_gil/attach, check for null returns and use unsafe casts to compare
the pointed-to Rust value, and add assertions mirroring the existing test to
validate the RustPython C-ABI implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6162606a-dd49-4b3f-9d34-c91288c4226f
📒 Files selected for processing (6)
.cspell.dict/python-more.txtcrates/capi/src/lib.rscrates/capi/src/pycapsule.rscrates/capi/src/util.rscrates/vm/src/builtins/capsule.rscrates/vm/src/vm/context.rs
✅ Files skipped from review due to trivial changes (1)
- .cspell.dict/python-more.txt
Summary by CodeRabbit