From 8682f578c1c8fd0486c886b001729907a5409a9f Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sun, 21 Aug 2016 17:41:56 +1000 Subject: [PATCH] Issue #27782: Fix m_methods handling in multiphase init Multi-phase extension module import now correctly allows the ``m_methods`` field to be used to add module level functions to instances of non-module types returned from ``Py_create_mod``. Patch by Xiang Zhang. --- Doc/c-api/module.rst | 2 +- Include/moduleobject.h | 2 +- .../test_importlib/extension/test_loader.py | 9 +++ Misc/ACKS | 1 + Misc/NEWS | 4 ++ Modules/_testmultiphase.c | 33 +++++++++- Objects/moduleobject.c | 64 ++++++++++--------- 7 files changed, 83 insertions(+), 32 deletions(-) diff --git a/Doc/c-api/module.rst b/Doc/c-api/module.rst index e810c69b1d8..7724350d3c5 100644 --- a/Doc/c-api/module.rst +++ b/Doc/c-api/module.rst @@ -324,7 +324,7 @@ The available slot types are: :c:type:`PyModule_Type`. Any type can be used, as long as it supports setting and getting import-related attributes. However, only ``PyModule_Type`` instances may be returned if the - ``PyModuleDef`` has non-*NULL* ``m_methods``, ``m_traverse``, ``m_clear``, + ``PyModuleDef`` has non-*NULL* ``m_traverse``, ``m_clear``, ``m_free``; non-zero ``m_size``; or slots other than ``Py_mod_create``. .. c:var:: Py_mod_exec diff --git a/Include/moduleobject.h b/Include/moduleobject.h index 229d7fadc24..b44fb9b961f 100644 --- a/Include/moduleobject.h +++ b/Include/moduleobject.h @@ -77,7 +77,7 @@ typedef struct PyModuleDef{ traverseproc m_traverse; inquiry m_clear; freefunc m_free; -}PyModuleDef; +} PyModuleDef; #ifdef __cplusplus } diff --git a/Lib/test/test_importlib/extension/test_loader.py b/Lib/test/test_importlib/extension/test_loader.py index 154a793ae86..8d20040aab8 100644 --- a/Lib/test/test_importlib/extension/test_loader.py +++ b/Lib/test/test_importlib/extension/test_loader.py @@ -212,6 +212,15 @@ class MultiPhaseExtensionModuleTests(abc.LoaderTests): self.assertNotEqual(type(mod), type(unittest)) self.assertEqual(mod.three, 3) + # issue 27782 + def test_nonmodule_with_methods(self): + '''Test creating a non-module object with methods defined''' + name = self.name + '_nonmodule_with_methods' + mod = self.load_module_by_name(name) + self.assertNotEqual(type(mod), type(unittest)) + self.assertEqual(mod.three, 3) + self.assertEqual(mod.bar(10, 1), 9) + def test_null_slots(self): '''Test that NULL slots aren't a problem''' name = self.name + '_null_slots' diff --git a/Misc/ACKS b/Misc/ACKS index 0664e872659..51c8d101d02 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1657,6 +1657,7 @@ Nickolai Zeldovich Yuxiao Zeng Uwe Zessin Cheng Zhang +Xiang Zhang Kai Zhu Tarek Ziadé Jelle Zijlstra diff --git a/Misc/NEWS b/Misc/NEWS index 1ecb6f1f94f..36af9741551 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,10 @@ Release date: TBA Core and Builtins ----------------- +- Issue #27782: Multi-phase extension module import now correctly allows the + ``m_methods`` field to be used to add module level functions to instances + of non-module types returned from ``Py_create_mod``. Patch by Xiang Zhang. + - Issue #27487: Warn if a submodule argument to "python -m" or runpy.run_module() is found in sys.modules after parent packages are imported, but before the submodule is executed. diff --git a/Modules/_testmultiphase.c b/Modules/_testmultiphase.c index 2005205d335..41da997af63 100644 --- a/Modules/_testmultiphase.c +++ b/Modules/_testmultiphase.c @@ -248,6 +248,7 @@ PyInit__testmultiphase(PyObject *spec) /**** Importing a non-module object ****/ static PyModuleDef def_nonmodule; +static PyModuleDef def_nonmodule_with_methods; /* Create a SimpleNamespace(three=3) */ static PyObject* @@ -255,7 +256,7 @@ createfunc_nonmodule(PyObject *spec, PyModuleDef *def) { PyObject *dct, *ns, *three; - if (def != &def_nonmodule) { + if (def != &def_nonmodule && def != &def_nonmodule_with_methods) { PyErr_SetString(PyExc_SystemError, "def does not match"); return NULL; } @@ -291,6 +292,36 @@ PyInit__testmultiphase_nonmodule(PyObject *spec) return PyModuleDef_Init(&def_nonmodule); } +PyDoc_STRVAR(nonmodule_bar_doc, +"bar(i,j)\n\ +\n\ +Return the difference of i - j."); + +static PyObject * +nonmodule_bar(PyObject *self, PyObject *args) +{ + long i, j; + long res; + if (!PyArg_ParseTuple(args, "ll:bar", &i, &j)) + return NULL; + res = i - j; + return PyLong_FromLong(res); +} + +static PyMethodDef nonmodule_methods[] = { + {"bar", nonmodule_bar, METH_VARARGS, nonmodule_bar_doc}, + {NULL, NULL} /* sentinel */ +}; + +static PyModuleDef def_nonmodule_with_methods = TEST_MODULE_DEF( + "_testmultiphase_nonmodule_with_methods", slots_create_nonmodule, nonmodule_methods); + +PyMODINIT_FUNC +PyInit__testmultiphase_nonmodule_with_methods(PyObject *spec) +{ + return PyModuleDef_Init(&def_nonmodule_with_methods); +} + /**** Non-ASCII-named modules ****/ static PyModuleDef def_nonascii_latin = { \ diff --git a/Objects/moduleobject.c b/Objects/moduleobject.c index a4cdc206c12..ac07642cfb0 100644 --- a/Objects/moduleobject.c +++ b/Objects/moduleobject.c @@ -130,6 +130,34 @@ check_api_version(const char *name, int module_api_version) return 1; } +static int +_add_methods_to_object(PyObject *module, PyObject *name, PyMethodDef *functions) +{ + PyObject *func; + PyMethodDef *fdef; + + for (fdef = functions; fdef->ml_name != NULL; fdef++) { + if ((fdef->ml_flags & METH_CLASS) || + (fdef->ml_flags & METH_STATIC)) { + PyErr_SetString(PyExc_ValueError, + "module functions cannot set" + " METH_CLASS or METH_STATIC"); + return -1; + } + func = PyCFunction_NewEx(fdef, (PyObject*)module, name); + if (func == NULL) { + return -1; + } + if (PyObject_SetAttrString(module, fdef->ml_name, func) != 0) { + Py_DECREF(func); + return -1; + } + Py_DECREF(func); + } + + return 0; +} + PyObject * PyModule_Create2(struct PyModuleDef* module, int module_api_version) { @@ -269,7 +297,7 @@ PyModule_FromDefAndSpec2(struct PyModuleDef* def, PyObject *spec, int module_api } } } else { - m = PyModule_New(name); + m = PyModule_NewObject(nameobj); if (m == NULL) { goto error; } @@ -297,7 +325,7 @@ PyModule_FromDefAndSpec2(struct PyModuleDef* def, PyObject *spec, int module_api } if (def->m_methods != NULL) { - ret = PyModule_AddFunctions(m, def->m_methods); + ret = _add_methods_to_object(m, nameobj, def->m_methods); if (ret != 0) { goto error; } @@ -331,7 +359,7 @@ PyModule_ExecDef(PyObject *module, PyModuleDef *def) return -1; } - if (PyModule_Check(module) && def->m_size >= 0) { + if (def->m_size >= 0) { PyModuleObject *md = (PyModuleObject*)module; if (md->md_state == NULL) { /* Always set a state pointer; this serves as a marker to skip @@ -387,37 +415,15 @@ PyModule_ExecDef(PyObject *module, PyModuleDef *def) int PyModule_AddFunctions(PyObject *m, PyMethodDef *functions) { - PyObject *name, *func; - PyMethodDef *fdef; - - name = PyModule_GetNameObject(m); + int res; + PyObject *name = PyModule_GetNameObject(m); if (name == NULL) { return -1; } - for (fdef = functions; fdef->ml_name != NULL; fdef++) { - if ((fdef->ml_flags & METH_CLASS) || - (fdef->ml_flags & METH_STATIC)) { - PyErr_SetString(PyExc_ValueError, - "module functions cannot set" - " METH_CLASS or METH_STATIC"); - Py_DECREF(name); - return -1; - } - func = PyCFunction_NewEx(fdef, (PyObject*)m, name); - if (func == NULL) { - Py_DECREF(name); - return -1; - } - if (PyObject_SetAttrString(m, fdef->ml_name, func) != 0) { - Py_DECREF(func); - Py_DECREF(name); - return -1; - } - Py_DECREF(func); - } + res = _add_methods_to_object(m, name, functions); Py_DECREF(name); - return 0; + return res; } int