Repository navigation
C API type slots - #8435
C API type slots#8435
Conversation
|
Caution Review failedThe pull request is closed.
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. |
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
CSlotstable (owned by heap types) and ac_new_trampolineto dispatchtp_newsupplied from C. - Threads
c_slotsinheritance/identity throughPyTypeSlotsand uses it in the “is not safe”__new__check. - Adds
PyType_GetSlot/Py_tp_newsupport incrates/capiwith tests, and reuses shared C-call utilities inmethodobject.
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.
| 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.
|
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 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 |
|
Thank you for the review! |
|
CPython semantics are “call base tp_new with subtype as argument”, not “call subtype’s current tp_new”. So we need a |
aaa9f3c to
2c3d5a8
Compare
|
After some experimenting with this, it makes me want fn get_c_tp_new<F>(
_: F,
) -> extern "C" fn(*mut PyTypeObject, *mut PyObject, *mut PyObject) -> *mut PyObject
where
F: FnStatic(PyTypeRef, FuncArgs, &VirtualMachine) -> PyResult,
{
extern "C" fn trampoline<F>(
subtype: *mut PyTypeObject,
args: *mut PyObject,
kwargs: *mut PyObject,
) -> *mut PyObject
where
F: FnStatic(PyTypeRef, FuncArgs, &VirtualMachine) -> PyResult,
{
unsafe { F::call_static(&*subtype.to_owned(), tuple_to_args(args), dict_to_kwargs(kwargs)) }
}
trampoline::<F>
}
let c_tp_new = ty.slots.new.load().map(|rust_tp_new| get_c_tp_new(rust_tp_new)I'm not sure how we can do this in todays ✨ rust. |
|
In todays rust we may have to use a static dispatch table. What do you think of this solution? fn get_c_tp_new<F: Any + Send + Sync>(
rust_tp_new: F,
) -> extern "C" fn(*mut PyTypeObject, *mut PyObject, *mut PyObject) -> *mut PyObject
where
F: Fn(PyTypeRef, FuncArgs, &VirtualMachine) -> PyResult,
{
extern "C" fn trampoline<F: Any + Send + Sync>(
subtype: *mut PyTypeObject,
args: *mut PyObject,
kwargs: *mut PyObject,
) -> *mut PyObject
where
F: Fn(PyTypeRef, FuncArgs, &VirtualMachine) -> PyResult,
{
let entry = SLOT_CACHE
.get(&TypeId::of::<F>())
.expect("could not find tp_new trampoline");
let func = entry.downcast_ref::<F>().expect("trampoline type mismatch");
with_vm(|vm| unsafe {
let args = FuncArgs::new(
tuple_to_args((&*args).try_downcast_ref(vm)?),
kwargs
.as_ref()
.map(|kwargs| dict_to_kwargs(vm, kwargs.try_downcast_ref::<PyDict>(vm)?))
.transpose()?
.unwrap_or_default(),
);
func((&*subtype).to_owned(), args, vm)
})
}
static SLOT_CACHE: LazyLock<DashMap<TypeId, Box<dyn Any + Send + Sync>>> =
LazyLock::new(DashMap::new);
SLOT_CACHE
.entry(TypeId::of::<F>())
.or_insert_with(|| Box::new(rust_tp_new));
trampoline::<F>
} |
2c3d5a8 to
965e4e0
Compare
|
@bschoenmaeckers Thank you so much! And I am sorry for late response. I am going to try your suggestion without table |
Merging this PR will not alter performance
Comparing Footnotes
|
d9f1e3a to
6ab3f65
Compare
There was a problem hiding this comment.
Could you move this to the object/pytype.rs file?
There was a problem hiding this comment.
Moved crates/capi/src/typeobject.rs into crates/capi/src/object/pytype.rs and dropped the extra typeobject module. Slot APIs are re-exported through object as before.
— commented by Grok 4.6
|
Please limit this to PyType_GetSlot only. I already have code for PySlot & PyType_Slot handling. |
| const ALIGN: usize = 16; | ||
|
|
||
| /// Instance payload whose body is a C-owned byte buffer of `basicsize` bytes. | ||
| pub struct PyCBody { |
There was a problem hiding this comment.
I don't think this will work. The last time I checked it is only possible for a type to have 1 Payload type with data in it. So for a C class extending a PyList, it needs both PyCBody & PyList holding data.
2fc0aca to
4c0b716
Compare
|
What is the status of this? |
|
@youknowone Do you have time to finish this? Or should I take it over? |
4c0b716 to
5c4325b
Compare
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
PyType_GetSlot reported Py_tp_new only for a function installed from C. A Rust slot such as int's or object's read as NULL, so an extension that reads its base type's tp_new to call it with its own subtype had nothing to call. Pair each Rust tp_new with a statically instantiated extern "C" function. `c_new_for::<S>` marshals `(subtype, args, kwds)`, calls `S::call`, and returns the object or NULL with the exception pending. `S` is `ViaConstructor<T>` for a `#[pyclass(with(Constructor))]` payload and `PythonNew` for `new_wrapper`; the target is fixed by the type parameter, so a subtype passed in runs the base's body. The derive macro emits a `HasStaticCSlots::TABLE` const per Constructor class holding the (Rust fn, C fn) pair, and stores a reference to it in the new `PyTypeSlots::static_c_slots` field on the defining type only. `PyType::c_tp_new` resolves a C-installed slot from `CSlots`, and a Rust slot by matching the slot's address against the tables along the MRO, then against the `new_wrapper` pair. An empty heap subclass of object now reports object's tp_new rather than NULL; the round-trip test is updated for that. Assisted-by: Claude Assisted-by: Grok
Reproduce what pyo3's PyNativeTypeInitializer does: a heap type whose C tp_new fetches the base type's tp_new through PyType_GetSlot and calls it with the subtype it was given. Cover object and int as bases, a Python-level subclass of the extension type, the slot each type reports, and a ValueError raised by the base crossing both trampolines. Replace a map/unwrap_or_else pair in a test helper that clippy flags. Assisted-by: Claude Assisted-by: Grok
Assisted-by: Grok:4.6
pyresult_to_ret_ptr stored the error with set_exception, which is the exc_info slot PUSH_EXC_INFO and POP_EXCEPT maintain. The C side reads the pending error with raised_exception and take_raised_exception, so a Rust tp_new reached through c_new_for returned NULL with nothing raised. Use set_raised_exception, and the same in the test helper that reports a missing base tp_new. Assisted-by: Claude
PyType_FromSlots left the Py_tp_new entry as a TODO. Hand it to set_tp_new once the type exists, so the type instantiates through the C function, gets the checked __new__ wrapper, and reports the function from PyType_GetSlot. A spec test covers those three paths. The pyo3 tests zero_sized_class and subclass_with_property were ignored; they pass now that PyType_GetSlot answers Py_tp_new for object and list, so the ignore markers are removed. rust_class and rust_class_with_member stay ignored because the spec asks for a non-zero basicsize. Assisted-by: Claude
set_tp_new now returns TypeError when the type's own dict already has __new__, matching its documented precondition. HeapTypeExt::c_slots is narrowed to pub(crate), and update_one_slot loads the type's C slot table once instead of twice. Add a capi test for the refusal and for the second installation, and a pyo3 test that a #[new] taking arguments runs from Python code, raises its error, and refuses a Python subclass. Assisted-by: Claude
5c4325b to
e5d33e4
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
@bschoenmaeckers could you check if this is a working design? tried not to increase runtime cost for non-capi path