C API type slots - #8435
Conversation
Sorry, something went wrong.
A METH_VARARGS | METH_KEYWORDS function received an empty dict when the call had no keywords, where the calling convention passes NULL. A callee that rejects keywords by testing the pointer saw a non-NULL kwargs. The METH_FASTCALL | METH_KEYWORDS path already passed a NULL kwnames. Name the four function pointer types the PyMethodPointer union holds instead of spelling out each signature inline. Assisted-by: Claude
The tp_new slot holds a Rust fn pointer, which a C `newfunc` cannot be. Put the C function in a `CSlots` table owned by the heap type it belongs to, and store `c_new_trampoline` in `new`; the trampoline reads the table off the type it is called with, so a subclass that inherited both reaches the same function and passes itself as `subtype`. `PyTypeSlots` holds one 8-byte pointer to the table rather than a field per C-provided slot, so further slots are a field in `CSlots` and a trampoline beside this one. The pointer is inherited with `new` by `set_new` and by `update_one_slot`, and dropped when a Python-level `__new__` replaces the slot. Compare what tp_new dispatches to, not the slot, in the "is not safe" check: every C type shares one trampoline, so comparing `new` alone let `CBase.__new__(CSub)` run CSub's tp_new. Move the C call marshalling from capi to `types::c_slots` so the trampoline and the METH_* call paths share it. Assisted-by: Claude
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: e94c101f-e799-45af-955e-8ea7a8255f5e You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file. Use the checkbox below for a quick retry:
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR adds initial support for C extension–provided type slots (starting with tp_new) by storing C ABI function pointers in a per-heap-type table and routing RustPython’s slot dispatch through a shared trampoline. It also exposes a minimal C-API surface to install and query these slots, plus tests validating inheritance and safety behavior.
Changes:
- Introduces a CSlots table (owned by heap types) and a c_new_trampoline to dispatch tp_new supplied from C.
- Threads c_slots inheritance/identity through PyTypeSlots and uses it in the “is not safe” __new__ check.
- Adds PyType_GetSlot / Py_tp_new support in crates/capi with tests, and reuses shared C-call utilities in methodobject.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file| File | Description |
|---|---|
| crates/vm/src/types/slot.rs | Adds c_slots tracking on PyTypeSlots, plus new_identity() and inheritance/override logic integration. |
| crates/vm/src/types/mod.rs | Exposes the new c_slots module and re-exports selected C-slot types/trampolines. |
| crates/vm/src/types/c_slots.rs | New module implementing C slot tables, trampolines, and shared argument/return conversion utilities. |
| crates/vm/src/builtins/type.rs | Stores owned C-slot tables on heap types and adjusts tp_new identity comparisons for safety checks. |
| crates/capi/src/typeobject.rs | New C-API glue for installing/querying tp_new via CSlots, with unit tests. |
| crates/capi/src/methodobject.rs | Reuses the shared (args, kwds) split + return conversion helpers for C function calls. |
| crates/capi/src/lib.rs | Registers the new typeobject module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| let owns_table = self.slots.c_slots.load().is_some() | ||
| && self.slots.c_slots.load() | ||
| != self.base.deref().and_then(|base| base.slots.c_slots.load()); | ||
| if owns_table { |
| /// The C slot table this type owns, once an extension has filled one in. | ||
| /// Types that inherit from it point at this same table. | ||
| pub c_slots: PyRwLock<Option<OwnedCSlots>>, | ||
| pub specialization_cache: TypeSpecializationCache, |
| /// Install a C `newfunc` as the type's tp_new. | ||
| /// | ||
| /// `ty` must be a heap type that does not already define `__new__` itself. | ||
| pub fn set_tp_new(vm: &VirtualMachine, ty: &Py<PyType>, tp_new: newfunc) -> PyResult<()> { |
| pub unsafe extern "C" fn PyType_GetSlot(ty: *const PyTypeObject, slot: c_int) -> *mut c_void { | ||
| let ty = unsafe { &*ty }; | ||
| let Some(c_slots) = ty.slots.c_slots() else { | ||
| return ptr::null_mut(); | ||
| }; | ||
| match CSlotId::from_raw(slot) { | ||
| Some(CSlotId::TpNew) => c_slots | ||
| .new | ||
| .load() | ||
| .map_or(ptr::null_mut(), |f| f as *mut c_void), | ||
| None => ptr::null_mut(), | ||
| } | ||
| } |
There was a problem hiding this comment.
This will not work. As PyType_GetSlot may be called on all types, so this will fail for builtin types. This is also required for extending classes because it requires fetching tp_new of the base type, which is almost always builtins.object.
Sorry, something went wrong.
|
I like the approach but this unfortunately does not work. When constructing heaptypes using the c-api, a type requires the tp_new of the base type. This is most of the time just object which we can fill in a c-slot trivially. To support extending all RustPython defined types we need to define the C tp_new slot for all types. Which might defeats the purpose separate slot definitions. Is there a way to make both C and Rust slots interchangeable? I tested your approach in this commit bc481ed. Which fails this assert in pyo3: https://github.com/PyO3/pyo3/blob/4df5ac6296fa9c9a375d874082a8ad90e43f9b2d/src/internal/pyclass_init.rs#L50 |
Sorry, something went wrong.
|
Thank you for the review! |
Sorry, something went wrong.
Reviewers
+1 more reviewerAt least 1 approving review is required to merge this pull request.
@bschoenmaeckers could you check if this is a working design? tried not to increase runtime cost for non-capi path