Merge pull request #145 from encukou/simplify-lazy-imports-c

Tests & nitpicks for the lazy import refactor

Author: pablogsal

Lib/test/test_lazy_import/__init__.py

@@ -13,7 +13,7 @@
 import contextlib
 
 from test import support
-from test.support.script_helper import assert_python_ok
+from test.support.script_helper import assert_python_ok, assert_python_failure
 
 try:
     import _testcapi
@@ -617,6 +617,14 @@ def test_dunder_lazy_import_invalid_arguments(self):
         with self.assertRaises(TypeError):
             __lazy_import__("sys", globals=1)
 
+        code = textwrap.dedent("""
+            __lazy_import__("sys", fromlist=(1, 2, 3))
+        """)
+        result = assert_python_failure("-c", code, NO_COLOR='y')
+        self.assertIn(
+            b"TypeError: Item in ``from list'' must be str, not int",
+            result.err)
+
     def test_dunder_lazy_import_builtins(self):
         """__lazy_import__ should use module's __builtins__ for __import__."""
         from test.test_lazy_import.data import dunder_lazy_import_builtins
@@ -766,6 +774,28 @@ def test_missing_lazy_from_import_shows_chained_traceback(self):
         """)
         assert_python_ok("-c", code)
 
+    @support.subTests('name', (
+        'test.test_lazy_import.data.broken_module_chained_cause',
+        'test.test_lazy_import.data.broken_module_chained_context',
+        'test.test_lazy_import.data.broken_module_chained_suppressed',
+    ))
+    def test_chained_exception_import_shows_notes(self, name):
+        """Accessing missing attribute from lazy from-import should chain errors."""
+        code = textwrap.dedent(f"""
+            lazy import {name}
+
+            try:
+                _ = test
+            except ValueError as e:
+                assert any(
+                    note.startswith("lazy import of '{name}' declared in ")
+                    for note in e.__notes__
+                ), e.__notes__
+            else:
+                raise AssertionError("ImportError was not raised")
+        """)
+        assert_python_ok("-c", code)
+
     def test_reification_retries_on_failure(self):
         """Failed reification should allow retry on subsequent access.
 

Lib/test/test_lazy_import/data/broken_module_chained_cause.py

@@ -0,0 +1,3 @@
+# Module that raises an exception with explicit cause during import
+cause = ValueError("Cause of failure")
+raise ValueError("This module always fails to import") from cause

Lib/test/test_lazy_import/data/broken_module_chained_context.py

@@ -0,0 +1,5 @@
+# Module that raises an exception with context during import
+try:
+    raise ValueError("Cause of failure")
+except:
+    raise ValueError("This module always fails to import")

Lib/test/test_lazy_import/data/broken_module_chained_suppressed.py

@@ -0,0 +1,2 @@
+# Module that raises an exception with suppressed context during import
+raise ValueError("This module always fails to import") from None

Objects/dictobject.c

@@ -2014,9 +2014,11 @@ static void
 replace_value(PyDictObject *mp, PyObject *key, Py_ssize_t ix,
               PyObject *old_value, PyObject *value)
 {
+    assert(can_modify_dict(mp));
+    assert(old_value != NULL);
+
     if (old_value != value) {
         _PyDict_NotifyEvent(PyDict_EVENT_MODIFIED, mp, key, value);
-        assert(old_value != NULL);
         if (DK_IS_UNICODE(mp->ma_keys)) {
             if (_PyDict_HasSplitTable(mp)) {
                 STORE_SPLIT_VALUE(mp, ix, value);
@@ -2031,7 +2033,9 @@ replace_value(PyDictObject *mp, PyObject *key, Py_ssize_t ix,
             STORE_VALUE(ep, value);
         }
     }
-    Py_DECREF(old_value); /* which **CAN** re-enter (see issue #22653) */
+    Py_DECREF(old_value); /* which **CAN** re-enter (see gh-66843) */
+
+    ASSERT_CONSISTENT(mp);
 }
 
 /*
@@ -2082,7 +2086,6 @@ insertdict(PyDictObject *mp,
     }
 
     replace_value(mp, key, ix, old_value, value);
-    ASSERT_CONSISTENT(mp);
     Py_DECREF(key);
     return 0;
 
@@ -3082,7 +3085,9 @@ _PyDict_ReplaceItemIf(PyObject *op, PyObject *key,
                       PyObject *expected, PyObject *replacement)
 {
     assert(PyDict_Check(op));
-    assert(expected != NULL && replacement != NULL);
+    assert(expected != NULL);
+    assert(replacement != NULL);
+
     Py_hash_t hash = PyObject_Hash(key);
     if (hash == -1) {
         return -1;
@@ -3098,7 +3103,6 @@ _PyDict_ReplaceItemIf(PyObject *op, PyObject *key,
     else if (current == expected) {
         // Do not look up the key again: equality can execute Python code.
         replace_value(mp, key, ix, current, Py_NewRef(replacement));
-        ASSERT_CONSISTENT(mp);
         result = 1;
     }
     Py_END_CRITICAL_SECTION();

Objects/lazyimportobject.c

@@ -16,8 +16,10 @@
 typedef struct {
     PyObject_HEAD
     PyObject *lz_builtins;  // Roots own the mapping; projections retain the root.
-    // A root stores its absolute name and original fromlist. A projection
-    // stores its source placeholder and the attribute to import from it.
+    // A root stores its absolute name (PyUnicode) in lz_from, and original
+    // fromlist in lz_attr.
+    // A projection stores its source placeholder (a PyLazyImportObject)
+    // in lz_from, and the attribute to import from (PyUnicode) it in lz_attr.
     PyObject *lz_from;
     PyObject *lz_attr;
     // Declaration location.
@@ -45,9 +47,18 @@ _PyLazyImport_New(_PyInterpreterFrame *frame, PyObject *builtins,
             "lazy_import: fromlist must be None, a string, or a tuple");
         return NULL;
     }
-    assert(PyLazyImport_CheckExact(name) ? builtins == NULL : builtins != NULL);
-    assert(!PyLazyImport_CheckExact(name) ||
-           (fromlist != NULL && PyUnicode_Check(fromlist)));
+#ifndef NDEBUG
+    if (PyLazyImport_CheckExact(name)) {
+        // projection
+        assert(builtins == NULL);
+        assert(fromlist != NULL);
+        assert(PyUnicode_Check(fromlist));
+    }
+    else {
+        // root
+        assert(builtins != NULL);
+    }
+#endif
     PyLazyImportObject *m = PyObject_GC_New(
         PyLazyImportObject, &PyLazyImport_Type);
     if (m == NULL) {
@@ -71,6 +82,7 @@ _PyLazyImport_New(_PyInterpreterFrame *frame, PyObject *builtins,
 
 // Reuse concrete attributes of initialized modules without waiting for imports
 // or resolving lazy attributes. Failed cache lookups are retried at resolution.
+// May return NULL with or without an exception set.
 static PyObject *
 lazy_import_get_loaded_attr(PyThreadState *tstate, PyObject *name,
                             PyObject *attr_name)
@@ -466,6 +478,7 @@ _PyImport_LoadLazyImportTstate(PyThreadState *tstate, PyObject *lazy_import)
 
 // Loading pkg.child can replace a placeholder in pkg.child with the module
 // before a from-import retrieves the value that belongs in that binding.
+// This is an optimization that can be safely skipped.
 static int
 lazy_import_replace_child(PyThreadState *tstate, PyObject *placeholder,
                           PyObject *name, PyObject *namespace,
@@ -563,7 +576,7 @@ lazy_import_resolve(PyObject *self, PyObject *args)
 static PyMethodDef lazy_import_methods[] = {
     {
         "resolve", lazy_import_resolve, METH_NOARGS,
-        PyDoc_STR("resolves the lazy import and returns the actual object")
+        PyDoc_STR("Resolve the lazy import and return the imported object.")
     },
     {NULL, NULL}
 };