Add capsule support to c-api#7940
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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
Summary by CodeRabbit