PyType_FromSpec*() overwrites the type's "__module__"

[scoder]

BPO	40703
Nosy	@ncoghlan, @scoder, @encukou, @serhiy-storchaka, @miss-islington, @shihai1991
PRs	bpo-40703: Let PyType_FromSpec() set "type.__module__" only if it is not set yet. #20273 [3.9] bpo-40703: Let PyType_FromSpec() set "type.__module__" only if it is not set yet. (GH-20273) #20782

^{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 = None
created_at = <Date 2020-05-20.21:24:32.273>
labels = ['expert-C-API', 'type-bug', '3.9', '3.10']
title = 'PyType_FromSpec*() overwrites the type\'s "__module__"'
updated_at = <Date 2020-10-20.11:48:55.197>
user = 'https://github.com/scoder'

bugs.python.org fields:

activity = <Date 2020-10-20.11:48:55.197>
actor = 'petr.viktorin'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['C API']
creation = <Date 2020-05-20.21:24:32.273>
creator = 'scoder'
dependencies = []
files = []
hgrepos = []
issue_num = 40703
keywords = ['patch']
message_count = 4.0
messages = ['369476', '371215', '371220', '379114']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'scoder', 'petr.viktorin', 'serhiy.storchaka', 'miss-islington', 'shihai1991']
pr_nums = ['20273', '20782']
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue40703'
versions = ['Python 3.9', 'Python 3.10']

[scoder]

The PyType_FromSpec() functions set the type's "__module__" attribute at the end:

cpython/Objects/typeobject.c

Lines 3154 to 3172 in 0509c45

	/* Set type.__module__ */
	s = strrchr(spec->name, '.');
	if (s != NULL) {
	int err;
	modname = PyUnicode_FromStringAndSize(
	spec->name, (Py_ssize_t)(s - spec->name));
	if (modname == NULL) {
	goto fail;
	}
	err = _PyDict_SetItemId(type->tp_dict, &PyId___module__, modname);
	Py_DECREF(modname);
	if (err != 0)
	goto fail;
	} else {
	if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1,
	"builtin type %.200s has no __module__ attribute",
	spec->name))
	goto fail;
	}

There are only two possible cases, either it finds a module name in the spec name and sets "__module__" from that, or it outputs a deprecation warning. Both behaviours are annoying because they ignore anything that the type already has in its dict, e.g. a property entry from the "members" or "getset" structs.

Since this code can't really be moved before the call to "PyType_Ready()" (which creates the type's dict and populates it), I think the best fix would be to first check if "__module__" is already in the dict and only otherwise take care of setting it.

I noticed this when trying to make the "__module__" attribute of Cython's coroutine type work with type specs, which should actually return the instance specific module name, i.e. the module that defines the coroutine, not the module that defines the type. This would normally be solved with an entry in "members", but that is discarded by the code above. While this approach means that the type does not have its own "__module__" entry, I think the use cases of pickling and documentation support it, because they care about the module name of the instance, not of the type.

I think working around this behaviour is somewhat easy by creating a new descriptor with PyDescr_NewMember() and re-adding it after calling PyType_FromSpec*(), so this is something that can just be fixed in Py3.9+.

Relevant tickets: bpo-15146 for setting "__module__" and bpo-20204 for the deprecation warning.

[scoder]

New changeset 24b8bad by scoder in branch 'master':
bpo-40703: Let PyType_FromSpec() set "type.__module__" only if it is not set yet. (GH-20273)
24b8bad

[scoder]

New changeset 9419158 by Miss Islington (bot) in branch '3.9':
bpo-40703: Let PyType_FromSpec() set "type.__module__" only if it is not set yet. (GH-20273) (GH-20782)
9419158

[encukou]

Thank you, Stefan!

This should have a test as well.
I'm willing to mentor someone who wants to get into the C-API, otherwise this has low priority for me.

[furkanonder]

@encukou This change has been in use for about 3 years without testing. Is there still a need to write a test?