PyModule_GetDict() claims it can never fail, but it can

[scoder]

BPO	12946
Nosy	@amauryfa, @scoder, @vstinner, @berkerpeksag, @matrixise
Files	issue12946.diff

^{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 2016-08-19.09:00:00.760>
created_at = <Date 2011-09-09.16:19:51.732>
labels = ['type-bug', 'docs']
title = 'PyModule_GetDict() claims it can never fail, but it can'
updated_at = <Date 2016-08-19.09:00:17.893>
user = 'https://github.com/scoder'

bugs.python.org fields:

activity = <Date 2016-08-19.09:00:17.893>
actor = 'berker.peksag'
assignee = 'docs@python'
closed = True
closed_date = <Date 2016-08-19.09:00:00.760>
closer = 'berker.peksag'
components = ['Documentation']
creation = <Date 2011-09-09.16:19:51.732>
creator = 'scoder'
dependencies = []
files = ['44030']
hgrepos = []
issue_num = 12946
keywords = ['patch']
message_count = 10.0
messages = ['143764', '143940', '143994', '144007', '227512', '272088', '272113', '273098', '273099', '273100']
nosy_count = 7.0
nosy_names = ['amaury.forgeotdarc', 'scoder', 'vstinner', 'docs@python', 'python-dev', 'berker.peksag', 'matrixise']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12946'
versions = ['Python 3.5', 'Python 3.6']

[scoder]

As is obvious from the code, PyModule_GetDict() can fail if being passed a non-module object, and when the (unlikely) dict creation at the end fails. The documentation of the C-API function should be fixed to reflect that, i.e. it should state that NULL is returned in the case of an error.

PyObject *
PyModule_GetDict(PyObject *m)
{
    PyObject *d;
    if (!PyModule_Check(m)) {
        PyErr_BadInternalCall();
        return NULL;
    }
    d = ((PyModuleObject *)m) -> md_dict;
    if (d == NULL)
        ((PyModuleObject *)m) -> md_dict = d = PyDict_New();
    return d;
}

[amauryfa]

The path with PyDict_New() is never taken, because PyModule_New already fills md_dict.

[vstinner]

So, can we close this issue?

[scoder]

I gave two reasons why this function can fail, and one turns out to be assumed-to-be-dead code. So, no, there are two issues now, one with the documentation, one with the code.

[vstinner]

I gave two reasons why this function can fail, and one turns out to be assumed-to-be-dead code.

If the call to PyDict_New() is never called, the test can be replaced with an assertion.

[berkerpeksag]

I've changed the dead code to

assert(d != NULL);

and added the following sentence to PyModule_GetDict documentation:

If *module* is not a module object (or a subtype of a module object),
:exc:`SystemError` is raised and *NULL* is returned.

[matrixise]

you can merge the patch.

[python-dev]

New changeset bd2a4c138b76 by Berker Peksag in branch '3.5':
Issue bpo-12946: Document that PyModule_GetDict can fail in some cases
https://hg.python.org/cpython/rev/bd2a4c138b76

New changeset c2e74b88947d by Berker Peksag in branch 'default':
Issue bpo-12946: Merge from 3.5
https://hg.python.org/cpython/rev/c2e74b88947d

[python-dev]

New changeset 4b64a049f451 by Berker Peksag in branch 'default':
Issue bpo-12946: Remove dead code in PyModule_GetDict
https://hg.python.org/cpython/rev/4b64a049f451

[berkerpeksag]

Thanks for the report, Stefan!