gh-157384: Unify _Py_ext_module_loader_info initialization (GH-157758)

- Unify `_Py_ext_module_loader_info_init*` functions. The “core” one was entirely unused, and the difference between “builtin” and “extension” can be expressed in arguments.
- Consolidate getting & checking the module name from spec. (This improves error message wording for `imp.create_builtin` with empty name.)
- Allow non-ASCII names.

Core modules (`sys` and `builtins`) still don't go through the routine.

Co-authored-by: Itamar Oren <itamarost@gmail.com>

Author: encukou

Include/internal/pycore_global_objects_fini_generated.h

@@ -262,6 +262,7 @@ struct _Py_global_strings {
         STRUCT_FOR_ID(_asyncio_future_blocking)
         STRUCT_FOR_ID(_blksize)
         STRUCT_FOR_ID(_bootstrap)
+        STRUCT_FOR_ID(_bootstrap_external)
         STRUCT_FOR_ID(_check_retval_)
         STRUCT_FOR_ID(_dealloc_warn)
         STRUCT_FOR_ID(_feature_version)

Include/internal/pycore_global_strings.h

@@ -51,9 +51,9 @@ typedef enum ext_module_kind {
 } _Py_ext_module_kind;
 
 typedef enum ext_module_origin {
-    _Py_ext_module_origin_CORE = 1,
-    _Py_ext_module_origin_BUILTIN = 2,
-    _Py_ext_module_origin_DYNAMIC = 3,
+    _Py_ext_module_origin_CORE = 1,  // 'sys' & 'builtins'
+    _Py_ext_module_origin_BUILTIN = 2,  // inittab or PyImport_CreateModuleFromInitfunc
+    _Py_ext_module_origin_DYNAMIC = 3,  // loaded dynamically
 } _Py_ext_module_origin;
 
 struct hook_prefixes {
@@ -79,21 +79,10 @@ struct _Py_ext_module_loader_info {
 extern void _Py_ext_module_loader_info_clear(
     struct _Py_ext_module_loader_info *info);
 extern int _Py_ext_module_loader_info_init(
-    struct _Py_ext_module_loader_info *info,
+    struct _Py_ext_module_loader_info *p_info,
     PyObject *name,
-    PyObject *filename,
+    PyObject *spec,
     _Py_ext_module_origin origin);
-extern int _Py_ext_module_loader_info_init_for_core(
-    struct _Py_ext_module_loader_info *p_info,
-    PyObject *name);
-extern int _Py_ext_module_loader_info_init_for_builtin(
-    struct _Py_ext_module_loader_info *p_info,
-    PyObject *name);
-#ifdef HAVE_DYNAMIC_LOADING
-extern int _Py_ext_module_loader_info_init_from_spec(
-    struct _Py_ext_module_loader_info *info,
-    PyObject *spec);
-#endif
 
 /* The result from running an extension module's init function.
  * Not used for modules defined via PyModExport (slots array).

Include/internal/pycore_importdl.h

@@ -293,6 +293,12 @@ def test_create_module_from_initfunc(self):
             "sp_pkg.sp_submod.__name__='sp_pkg.sp_submod'\n"
             "sys.modules[\"sp_pkg.sp_submod\"] is sp_pkg.sp_submod=True\n"
             "\"sp_submod\" in sys.modules=False\n"
+            # Module with non-ASCII name. (The SystemError shows the internal
+            # punycode-encoded name: not ideal, but also not horrible.)
+            "created nonascii_mp_\N{SNAKE}: in sys.modules=False\n"
+            "ascii(mp.__name__)=\"'nonascii_mp_\\\\U0001f40d'\" mp.executed='yes'\n"
+            "SystemError: 'initialization of nonascii_sp__rf17k "
+            "did not return PyModuleDef'\n"
         )
 
     def test_inittab_submodule_multiphase(self):

Include/internal/pycore_runtime_init_generated.h

@@ -1264,6 +1264,13 @@ class Spec2:
                 origin = "a\x00b"
             _imp.create_dynamic(Spec2())
 
+
+    def test_create_dynamic_no_origin(self):
+        with self.assertRaisesRegex(AttributeError, "no attribute 'origin'"):
+            class Spec:
+                name = "abc"
+            _imp.create_dynamic(Spec())
+
     def test_create_builtin(self):
         class Spec:
             pass
@@ -1273,7 +1280,7 @@ class Spec:
         self.assertIs(_imp.create_builtin(spec), sys)
 
         spec.name = None
-        with self.assertRaisesRegex(TypeError, 'name must be string, not NoneType'):
+        with self.assertRaisesRegex(TypeError, 'module name must be a string, not NoneType'):
             _imp.create_builtin(spec)
 
         # gh-142029
@@ -1283,7 +1290,7 @@ class Spec:
 
         # gh-142029
         spec.name = ""
-        with self.assertRaisesRegex(ValueError, 'name must not be empty'):
+        with self.assertRaisesRegex(ValueError, 'module name must not be empty'):
             _imp.create_builtin(spec)
 
     def test_filter_syntax_warnings_by_module(self):

Include/internal/pycore_unicodeobject_generated.h

@@ -0,0 +1,3 @@
+:c:func:`PyImport_CreateModuleFromInitfunc` now accepts non-ASCII module
+names for multi-phase init modules, instead of raising
+:exc:`UnicodeEncodeError`.

Lib/test/test_embed.py

@@ -2460,6 +2460,40 @@ PyInit_cmfi_sp_pkg(void)
     return PyModuleDef_Init(&cmfi_sp_pkg_def);
 }
 
+
+// Modules with non-ASCII names: multi-phase init is supported,
+// single-phase init is not.
+static PyModuleDef_Slot cmfi_nonascii_mp_slots[] = {
+    {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED},
+    {Py_mod_gil, Py_MOD_GIL_NOT_USED},
+    {0, NULL},
+};
+
+static PyModuleDef cmfi_nonascii_mp_def = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "nonascii_mp_\xf0\x9f\x90\x8d",
+    .m_size = 0,
+    .m_slots = cmfi_nonascii_mp_slots,
+};
+
+static PyObject*
+PyInit_cmfi_nonascii_mp(void)
+{
+    return PyModuleDef_Init(&cmfi_nonascii_mp_def);
+}
+
+static PyModuleDef cmfi_nonascii_sp_def = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "nonascii_sp_\xf0\x9f\x90\x8d",
+    .m_size = -1,
+};
+
+static PyObject*
+PyInit_cmfi_nonascii_sp(void)
+{
+    return PyModule_Create(&cmfi_nonascii_sp_def);
+}
+
 static PyObject*
 create_module(PyObject* self, PyObject* spec)
 {
@@ -2487,6 +2521,14 @@ create_module(PyObject* self, PyObject* spec)
         Py_DECREF(name);
         return PyImport_CreateModuleFromInitfunc(spec, PyInit_cmfi_sp_submod);
     }
+    if (PyUnicode_EqualToUTF8(name, "nonascii_mp_\xf0\x9f\x90\x8d")) {
+        Py_DECREF(name);
+        return PyImport_CreateModuleFromInitfunc(spec, PyInit_cmfi_nonascii_mp);
+    }
+    if (PyUnicode_EqualToUTF8(name, "nonascii_sp_\xf0\x9f\x90\x8d")) {
+        Py_DECREF(name);
+        return PyImport_CreateModuleFromInitfunc(spec, PyInit_cmfi_nonascii_sp);
+    }
     PyErr_Format(PyExc_LookupError, "static module %R not found", name);
     Py_DECREF(name);
     return NULL;
@@ -2548,6 +2590,11 @@ test_create_module_from_initfunc(void)
         L"print(f'{sp_pkg.sp_submod.__name__=}');"
         L"print(f'{sys.modules[\"sp_pkg.sp_submod\"] is sp_pkg.sp_submod=}');"
         L"print(f'{\"sp_submod\" in sys.modules=}');"
+        // Non-ASCII names: multi-phase init works, single-phase init doesn't
+        L"import importlib;"
+        L"mp = importlib.import_module('nonascii_mp_\\N{SNAKE}');"
+        L"print(f'{ascii(mp.__name__)=} {mp.executed=}');"
+        L"try_import('nonascii_sp_\\N{SNAKE}');"
     };
     PyConfig config;
     if (PyImport_AppendInittab("create_static_module",
@@ -2568,7 +2615,10 @@ test_create_module_from_initfunc(void)
         "   @classmethod\n"
         "   def find_spec(cls, fullname, path, target=None):\n"
         "       if fullname in {'my_test_extension', 'embedded_ext',\n"
-        "                       'sp_pkg', 'sp_pkg.sp_submod'}:\n"
+        "                       'sp_pkg', 'sp_pkg.sp_submod',\n"
+        "                       'nonascii_mp_\\N{SNAKE}',\n"
+        "                       'nonascii_sp_\\N{SNAKE}',\n"
+        "       }:\n"
         "           return spec_from_loader(fullname, cls, origin=cls._ORIGIN,\n"
         "                                   is_package=(fullname == 'sp_pkg'))\n"
         "       return None\n"
@@ -2583,6 +2633,13 @@ test_create_module_from_initfunc(void)
         "       create_static_module.exec_module(module)\n"
         "       module.executed = 'yes'\n"
         "sys.meta_path.append(StaticExtensionImporter)\n"
+        "def try_import(name):\n"
+        "   try:\n"
+        "       importlib.import_module(name)\n"
+        "   except SystemError as exc:\n"
+        "       print(f'SystemError: {ascii(str(exc))}')\n"
+        "   else:\n"
+        "       print(f'no SystemError for {ascii(name)}!')\n"
     );
     if (result < 0) {
         fprintf(stderr, "PyRun_SimpleString() failed\n");

Lib/test/test_import/__init__.py

@@ -2507,27 +2507,16 @@ lookup_inittab_entry(const struct _Py_ext_module_loader_info* info)
 
 static PyObject*
 create_builtin(
-    PyThreadState *tstate, PyObject *name,
+    PyThreadState *tstate,
+    PyObject *name, /* if NULL, get from spec */
     PyObject *spec,
     PyModInitFunction initfunc)
 {
     struct _Py_ext_module_loader_info info;
-    if (_Py_ext_module_loader_info_init_for_builtin(&info, name) < 0) {
+    _Py_ext_module_origin origin = _Py_ext_module_origin_BUILTIN;
+    if (_Py_ext_module_loader_info_init(&info, name, spec, origin) < 0) {
         return NULL;
     }
-    if (initfunc != NULL) {
-        /* An explicitly provided init function (see
-         * PyImport_CreateModuleFromInitfunc()) may belong to a submodule
-         * whose single-phase init creates the module using only the last
-         * component of the name (as e.g. pybind11 does).  Set the package
-         * context so that PyModule_Create() resolves the full name, the
-         * same as for dynamically loaded extensions. */
-        info.newcontext = PyUnicode_AsUTF8(info.name);
-        if (info.newcontext == NULL) {
-            _Py_ext_module_loader_info_clear(&info);
-            return NULL;
-        }
-    }
 
     struct extensions_cache_value *cached = NULL;
     PyObject *mod = import_find_extension(tstate, &info, &cached);
@@ -2558,7 +2547,7 @@ create_builtin(
         struct _inittab *entry = lookup_inittab_entry(&info);
         if (entry == NULL) {
             mod = NULL;
-            _PyErr_SetModuleNotFoundError(name);
+            _PyErr_SetModuleNotFoundError(info.name);
             goto finally;
         }
 
@@ -2610,20 +2599,7 @@ PyImport_CreateModuleFromInitfunc(
 
     PyThreadState *tstate = _PyThreadState_GET();
 
-    PyObject *name = PyObject_GetAttr(spec, &_Py_ID(name));
-    if (name == NULL) {
-        return NULL;
-    }
-
-    if (!PyUnicode_Check(name)) {
-        PyErr_Format(PyExc_TypeError,
-                     "spec name must be string, not %T", name);
-        Py_DECREF(name);
-        return NULL;
-    }
-
-    PyObject *mod = create_builtin(tstate, name, spec, initfunc);
-    Py_DECREF(name);
+    PyObject *mod = create_builtin(tstate, NULL, spec, initfunc);
     return mod;
 }
 
@@ -2835,8 +2811,8 @@ PyImport_ExecCodeModuleWithPathnames(const char *name, PyObject *co,
             Py_FatalError("no current interpreter");
         }
 
-        external= PyObject_GetAttrString(IMPORTLIB(interp),
-                                         "_bootstrap_external");
+        external= PyObject_GetAttr(IMPORTLIB(interp),
+                                   &_Py_ID(_bootstrap_external));
         if (external != NULL) {
             pathobj = PyObject_CallMethodOneArg(
                 external, &_Py_ID(_get_sourcefile), cpathobj);
@@ -2920,8 +2896,8 @@ PyImport_ExecCodeModuleObject(PyObject *name, PyObject *co, PyObject *pathname,
     if (pathname == NULL) {
         pathname = ((PyCodeObject *)co)->co_filename;
     }
-    external = PyObject_GetAttrString(IMPORTLIB(tstate->interp),
-                                      "_bootstrap_external");
+    external = PyObject_GetAttr(IMPORTLIB(tstate->interp),
+                                &_Py_ID(_bootstrap_external));
     if (external == NULL) {
         Py_DECREF(d);
         return NULL;
@@ -3499,8 +3475,8 @@ PyObject *
 _PyImport_GetImportlibExternalLoader(PyInterpreterState *interp,
                                      const char *loader_name)
 {
-    PyObject *bootstrap = PyObject_GetAttrString(IMPORTLIB(interp),
-                                                 "_bootstrap_external");
+    PyObject *bootstrap = PyObject_GetAttr(IMPORTLIB(interp),
+                                           &_Py_ID(_bootstrap_external));
     if (bootstrap == NULL) {
         return NULL;
     }
@@ -3513,8 +3489,8 @@ _PyImport_GetImportlibExternalLoader(PyInterpreterState *interp,
 PyObject *
 _PyImport_BlessMyLoader(PyInterpreterState *interp, PyObject *module_globals)
 {
-    PyObject *external = PyObject_GetAttrString(IMPORTLIB(interp),
-                                                "_bootstrap_external");
+    PyObject *external = PyObject_GetAttr(IMPORTLIB(interp),
+                                          &_Py_ID(_bootstrap_external));
     if (external == NULL) {
         return NULL;
     }
@@ -5085,27 +5061,7 @@ _imp_create_builtin(PyObject *module, PyObject *spec)
 {
     PyThreadState *tstate = _PyThreadState_GET();
 
-    PyObject *name = PyObject_GetAttrString(spec, "name");
-    if (name == NULL) {
-        return NULL;
-    }
-
-    if (!PyUnicode_Check(name)) {
-        PyErr_Format(PyExc_TypeError,
-                     "name must be string, not %.200s",
-                     Py_TYPE(name)->tp_name);
-        Py_DECREF(name);
-        return NULL;
-    }
-
-    if (PyUnicode_GetLength(name) == 0) {
-        PyErr_Format(PyExc_ValueError, "name must not be empty");
-        Py_DECREF(name);
-        return NULL;
-    }
-
-    PyObject *mod = create_builtin(tstate, name, spec, NULL);
-    Py_DECREF(name);
+    PyObject *mod = create_builtin(tstate, NULL, spec, NULL);
     return mod;
 }
 
@@ -5437,7 +5393,8 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file)
     PyThreadState *tstate = _PyThreadState_GET();
 
     struct _Py_ext_module_loader_info info;
-    if (_Py_ext_module_loader_info_init_from_spec(&info, spec) < 0) {
+    _Py_ext_module_origin origin = _Py_ext_module_origin_DYNAMIC;
+    if (_Py_ext_module_loader_info_init(&info, NULL, spec, origin) < 0) {
         return NULL;
     }