Issue python#27782: Fix m_methods handling in multiphase init

ncoghlan · ncoghlan · commit 8682f578c1c8 · 2016-08-21T17:41:56.000+10:00
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.
diff --git 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
@@ -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
@@ -212,6 +212,15 @@ def test_nonmodule(self):
         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
@@ -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
@@ -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
@@ -248,14 +248,15 @@ 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*
 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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ typedef struct PyModuleDef{`
`77`	`77`	`traverseproc m_traverse;`
`78`	`78`	`inquiry m_clear;`
`79`	`79`	`freefunc m_free;`
`80`		`-}PyModuleDef;`
	`80`	`+} PyModuleDef;`
`81`	`81`
`82`	`82`	`#ifdef __cplusplus`
`83`	`83`	`}`