gh-119560: Drop an Invalid Assert in PyState_FindModule() (gh-119561)

The assertion was added in gh-118532 but was based on the invalid assumption that PyState_FindModule() would only be called with an already-initialized module def.  I've added a test to make sure we don't make that assumption again.

diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py
index 64282d0f2d0bcf6ff4ecc1bcbbb664222149808e..11eaae5e47e97ac21b2613abc9782946f12388a6 100644 (file)
--- a/Lib/test/test_import/__init__.py
+++ b/Lib/test/test_import/__init__.py
@@ -2887,6 +2887,13 @@ class SinglephaseInitTests(unittest.TestCase):
 
                 self.assertIs(reloaded.snapshot.cached, reloaded.module)
 
+    def test_check_state_first(self):
+        for variant in ['', '_with_reinit', '_with_state']:
+            name = f'{self.NAME}{variant}_check_cache_first'
+            with self.subTest(name):
+                mod = self._load_dynamic(name, self.ORIGIN)
+                self.assertEqual(mod.__name__, name)
+
     # Currently, for every single-phrase init module loaded
     # in multiple interpreters, those interpreters share a
     # PyModuleDef for that object, which can be a problem.

diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-25-12-52-25.gh-issue-119560.wSlm8q.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-25-12-52-25.gh-issue-119560.wSlm8q.rst
new file mode 100644 (file)
index 0000000..3a28a94
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2024-05-25-12-52-25.gh-issue-119560.wSlm8q.rst
@@ -0,0 +1,3 @@
+An invalid assert in beta 1 has been removed.  The assert would fail if
+``PyState_FindModule()`` was used in an extension module's init function
+before the module def had been initialized.

diff --git a/Modules/_testsinglephase.c b/Modules/_testsinglephase.c
index 448be502466e79942c65649da9ac4419b5ee7643..bcdb5ba31842fd7e5909910e083172b0a5db4a65 100644 (file)
--- a/Modules/_testsinglephase.c
+++ b/Modules/_testsinglephase.c
@@ -1,7 +1,7 @@
 
 /* Testing module for single-phase initialization of extension modules
 
-This file contains 5 distinct modules, meaning each as its own name
+This file contains 8 distinct modules, meaning each as its own name
 and its own init function (PyInit_...).  The default import system will
 only find the one matching the filename: _testsinglephase.  To load the
 others you must do so manually.  For example:
@@ -14,7 +14,7 @@ spec = importlib.util.spec_from_file_location(name, filename, loader=loader)
 mod = importlib._bootstrap._load(spec)
 ```
 
-Here are the 5 modules:
+Here are the 8 modules:
 
 * _testsinglephase
    * def: _testsinglephase_basic,
@@ -136,6 +136,32 @@ Here are the 5 modules:
       5. increment <global>.initialized_count
    * functions: see common functions below
    * import system: same as _testsinglephase_basic_copy
+* _testsinglephase_check_cache_first
+   * def: _testsinglepahse_check_cache_first
+      * m_name: "_testsinglephase_check_cache_first"
+      * m_size: -1
+   * state: none
+   * init function:
+      * tries PyState_FindModule() first
+      * otherwise creates empty module
+   * functions: none
+   * import system: same as _testsinglephase
+* _testsinglephase_with_reinit_check_cache_first
+   * def: _testsinglepahse_with_reinit_check_cache_first
+      * m_name: "_testsinglephase_with_reinit_check_cache_first"
+      * m_size: 0
+   * state: none
+   * init function: same as _testsinglephase_check_cache_first
+   * functions: none
+   * import system: same as _testsinglephase_with_reinit
+* _testsinglephase_with_state_check_cache_first
+   * def: _testsinglepahse_with_state_check_cache_first
+      * m_name: "_testsinglephase_with_state_check_cache_first"
+      * m_size: 42
+   * state: none
+   * init function: same as _testsinglephase_check_cache_first
+   * functions: none
+   * import system: same as _testsinglephase_with_state
 
 Module state:
 
@@ -650,3 +676,64 @@ PyInit__testsinglephase_with_state(void)
 finally:
     return module;
 }
+
+
+/****************************************************/
+/* the _testsinglephase_*_check_cache_first modules */
+/****************************************************/
+
+static struct PyModuleDef _testsinglephase_check_cache_first = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "_testsinglephase_check_cache_first",
+    .m_doc = PyDoc_STR("Test module _testsinglephase_check_cache_first"),
+    .m_size = -1,  // no module state
+};
+
+PyMODINIT_FUNC
+PyInit__testsinglephase_check_cache_first(void)
+{
+    assert(_testsinglephase_check_cache_first.m_base.m_index == 0);
+    PyObject *mod = PyState_FindModule(&_testsinglephase_check_cache_first);
+    if (mod != NULL) {
+        return Py_NewRef(mod);
+    }
+    return PyModule_Create(&_testsinglephase_check_cache_first);
+}
+
+
+static struct PyModuleDef _testsinglephase_with_reinit_check_cache_first = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "_testsinglephase_with_reinit_check_cache_first",
+    .m_doc = PyDoc_STR("Test module _testsinglephase_with_reinit_check_cache_first"),
+    .m_size = 0,  // no module state
+};
+
+PyMODINIT_FUNC
+PyInit__testsinglephase_with_reinit_check_cache_first(void)
+{
+    assert(_testsinglephase_with_reinit_check_cache_first.m_base.m_index == 0);
+    PyObject *mod = PyState_FindModule(&_testsinglephase_with_reinit_check_cache_first);
+    if (mod != NULL) {
+        return Py_NewRef(mod);
+    }
+    return PyModule_Create(&_testsinglephase_with_reinit_check_cache_first);
+}
+
+
+static struct PyModuleDef _testsinglephase_with_state_check_cache_first = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "_testsinglephase_with_state_check_cache_first",
+    .m_doc = PyDoc_STR("Test module _testsinglephase_with_state_check_cache_first"),
+    .m_size = 42,  // not used
+};
+
+PyMODINIT_FUNC
+PyInit__testsinglephase_with_state_check_cache_first(void)
+{
+    assert(_testsinglephase_with_state_check_cache_first.m_base.m_index == 0);
+    PyObject *mod = PyState_FindModule(&_testsinglephase_with_state_check_cache_first);
+    if (mod != NULL) {
+        return Py_NewRef(mod);
+    }
+    return PyModule_Create(&_testsinglephase_with_state_check_cache_first);
+}

diff --git a/Python/import.c b/Python/import.c
index ba44477318d4731768dbc915783994e7f1001654..4f3325aa67bd0a5164ad1859f34e9c7ed1871371 100644 (file)
--- a/Python/import.c
+++ b/Python/import.c
@@ -457,7 +457,6 @@ static Py_ssize_t
 _get_module_index_from_def(PyModuleDef *def)
 {
     Py_ssize_t index = def->m_base.m_index;
-    assert(index > 0);
 #ifndef NDEBUG
     struct extensions_cache_value *cached = _find_cached_def(def);
     assert(cached == NULL || index == _get_cached_module_index(cached));
@@ -489,7 +488,7 @@ _set_module_index(PyModuleDef *def, Py_ssize_t index)
 static const char *
 _modules_by_index_check(PyInterpreterState *interp, Py_ssize_t index)
 {
-    if (index == 0) {
+    if (index <= 0) {
         return "invalid module index";
     }
     if (MODULES_BY_INDEX(interp) == NULL) {