PyType_FromSpec() should accept tp_doc=NULL

[vstinner]

BPO	41832
Nosy	@vstinner, @encukou, @corona10, @shihai1991, @Fidget-Spinner
PRs	bpo-41832: PyType_FromModuleAndSpec() can accept the NULL tp_doc slot. #23123 bpo-41832: Update PyType_Slot.pfunc description info in type.rst #23243 [3.9] bpo-41832, bpo-42083: PyType_FromModuleAndSpec() can accept the NULL tp_doc slot (GH-25687) #25852

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-11-15.09:09:14.483>
created_at = <Date 2020-09-22.10:29:31.974>
labels = ['interpreter-core', '3.10']
title = 'PyType_FromSpec() should accept tp_doc=NULL'
updated_at = <Date 2021-05-03.14:06:50.277>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2021-05-03.14:06:50.277>
actor = 'kj'
assignee = 'none'
closed = True
closed_date = <Date 2020-11-15.09:09:14.483>
closer = 'shihai1991'
components = ['Interpreter Core']
creation = <Date 2020-09-22.10:29:31.974>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 41832
keywords = ['patch']
message_count = 13.0
messages = ['377308', '380238', '380302', '380316', '380452', '380453', '380488', '380709', '380767', '380779', '380976', '381006', '381022']
nosy_count = 5.0
nosy_names = ['vstinner', 'petr.viktorin', 'corona10', 'shihai1991', 'kj']
pr_nums = ['23123', '23243', '25852']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue41832'
versions = ['Python 3.10']

[vstinner]

When a Python type is defined in a C extension module as a static type, its documentation can be a NULL string (NULL pointer). But PyType_FromSpec() does crash if tp_doc is NULL, since this change:

commit 032400b
Author: Georg Brandl <georg@python.org>
Date: Sat Feb 19 21:47:02 2011 +0000

bpo-11249: in PyType_FromSpec, copy tp_doc slot since it usually will point to a static string literal which should not be deallocated together with the type.

(...)
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index e9c7591b81..b1fe44ebe4 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -2347,6 +2347,17 @@ PyObject* PyType_FromSpec(PyType_Spec *spec)
            goto fail;
        }
        *(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc;
+
+        /* need to make a copy of the docstring slot, which usually
+           points to a static string literal */
+        if (slot->slot == Py_tp_doc) {
+            ssize_t len = strlen(slot->pfunc)+1;
+            char *tp_doc = PyObject_MALLOC(len);
+            if (tp_doc == NULL)
+               goto fail;
+            memcpy(tp_doc, slot->pfunc, len);
+            res->ht_type.tp_doc = tp_doc;
+        }
     }
 
     return (PyObject*)res;

I propose to accept tp_doc=NULL in PyType_FromSpec(), as we do in static types.

I noticed this difference while reviewing PR 22220 which convert _lsprof extension static types to heap types.

[shihai1991]

I will take a look in this weekend :)

[encukou]

The PyType_Slot documentation says that the pointer may not be NULL: https://docs.python.org/3/c-api/type.html?highlight=pytype_fromspec#c.PyType_Slot.PyType_Slot.pfunc

If you change this, why do it only for tp_doc, but for all the slots? NULL should *always* mean that the slot is set to NULL instead of inherited. (Except maybe in cases where this is dangerous; then it should result in an error?)
See: https://bugs.python.org/issue26979

If you want to only change this for tp_doc, please also update the PyType_Slot documentation to cover the exception.

[shihai1991]

If you change this, why do it only for tp_doc, but for all the slots? NULL should *always* mean that the slot is set to NULL instead of inherited. (Except maybe in cases where this is dangerous; then it should result in an error?

IMO, it's a proper user case.
From bpo-26978: "I'll also try making an explicit {Py_tp_dealloc, NULL} override the inherited value, as per Serhiy's suggestion."
Petr, do you have a PR about it?

If you want to only change this for tp_doc, please also update the PyType_Slot documentation to cover the exception.

Copy that, I will update it soon.

[vstinner]

New changeset 88c2cfd by Hai Shi in branch 'master':
bpo-41832: PyType_FromModuleAndSpec() now accepts NULL tp_doc (GH-23123)
88c2cfd

[vstinner]

Thanks Hai Shi.

[shihai1991]

Thank you all:)

[encukou]

The PR entirely removed the note that a slot's value "May not be NULL".
In fact, only tp_doc may be NULL. AFAIK, the restriction is still there for other slots.
Can that note be added back?