PEP 697 -- Limited C API for Extending Opaque Types

[encukou]

PEP 697 was accepted and needs to be implemented.

Linked PRs

• gh-103509: PEP 697 -- Limited C API for Extending Opaque Types #103511

[encukou]

Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.

[erlend-aasland]

Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.

I'll just post my suggestion here instead of opening a PR. It kind of bothers me a bit that PyObject_GetTypeData and PyType_GetTypeDataSize currently always succeeds (and thus never raise an exception), but both are documented to raise an exception on error. I feel the urge to assert the current behaviour so we suddenly don't end up with them returning NULL or -1 without an exception set. What do you think, Petr, is it worth it?

suggestion - diff

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index 171c76a59a..ad7b9c1ce4 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -4532,7 +4532,12 @@ void *
 PyObject_GetTypeData(PyObject *obj, PyTypeObject *cls)
 {
     assert(PyObject_TypeCheck(obj, cls));
-    return (char *)obj + _align_up(cls->tp_base->tp_basicsize);
+    void *ret = (char *)obj + _align_up(cls->tp_base->tp_basicsize);
+
+    /* This API is documented to raise an exception if it returns NULL.
+     * The current implementation, however, always succeeds. */
+    assert(ret != NULL);
+    return ret;
 }
 
 Py_ssize_t
@@ -4542,6 +4547,10 @@ PyType_GetTypeDataSize(PyTypeObject *cls)
     if (result < 0) {
         return 0;
     }
+
+    /* This API is documented to raise an exception if it returns -1.
+     * The current implementation, however, always succeeds. */
+     assert(result >= 0);
     return result;
 }

[sunmy2019]

I feel the urge to assert the current behaviour

makes sense

[encukou]

It doesn't bother me.
(char *)obj + _align_up(cls->tp_base->tp_basicsize) can obviously never be NULL, so the assert is superfluous. If a future dev sets it to something that can be NULL, they're responsible for also setting an exception -- as always when adding an error case. The fact that there's no existing error case looks irrelevant to me.

We could add a comment for clarity, but I would word it as.

/* The user must ensure that *cls* was created using
   negative PyType_Spec.basicsize. Currently we don't have a good way to check that.
   For errors that we can detect, this function should raise an exception and return NULL. */

However, that's just repeating what's already in the docs...

[erlend-aasland]

NP, it's not a deal-breaker for me. Let's just leave it as it is 🙂