Fixes NameError bug

Kronuz · facebook-github-bot · commit ff52c1d9d807 · 2023-09-20T03:27:49.000-07:00
Summary:
We had been seeing bogus `NameError` exceptions and uninformative `ImportError: Unable to resolve all lazy imports` when import cycles got in the way of resolving a deferred objects. The reason was `_PyImport_LoadLazyImportTstate()` returned `NULL` without error to signal a cycle and this wasn't handled properly in all cases.

We now return an explicit `ImportCycleError` instead, and ignore that particular exception during `LookupAttr()` to make the import system resolve cases like `from . import version`, where `version` is a submodule.

Reviewed By: cxxxs

Differential Revision: D49166179

fbshipit-source-id: 0423e72865cdebbb4f7b8142527306aae06756c8
diff --git a/Cinder/Include/cinder/exports.h b/Cinder/Include/cinder/exports.h
@@ -24,6 +24,7 @@ CiAPI_FUNC(PyObject *) Ci_dict_subscript(PyObject *mp, PyObject *key);
 CiAPI_FUNC(PyObject *) Ci_list_subscript(PyObject *list, PyObject *item);
 CiAPI_FUNC(PyObject *) Ci_tuple_subscript(PyObject *self, PyObject *item);
 CiAPI_FUNC(PyObject *) Ci_module_lookupattro(PyObject *self, PyObject *name, int suppress);
+CiAPI_FUNC(PyObject *) Ci_strictmodule_lookupattro(PyObject *self, PyObject *name, int suppress);
 
 CiAPI_FUNC(Py_hash_t) Ci_TupleHashItems(PyObject *const *items, Py_ssize_t len);
 
diff --git a/CinderX/known-core-python-exported-symbols b/CinderX/known-core-python-exported-symbols
@@ -46,6 +46,7 @@ Ci_PyWaitHandle_Type
 Ci_RuntimeState_SetProfileInterpPeriod
 Ci_set_attribute_error_context
 Ci_StaticFunction_Vectorcall
+Ci_strictmodule_lookupattro
 Ci_SuperLookupMethodOrAttr
 Ci_ThreadState_SetProfileInterp
 Ci_ThreadState_SetProfileInterpAll
@@ -756,6 +757,7 @@ PyExc_FileNotFoundError
 PyExc_FloatingPointError
 PyExc_FutureWarning
 PyExc_GeneratorExit
+PyExc_ImportCycleError
 PyExc_ImportError
 PyExc_ImportWarning
 PyExc_IndentationError
diff --git a/Include/pyerrors.h b/Include/pyerrors.h
@@ -79,6 +79,7 @@ PyAPI_DATA(PyObject *) PyExc_BufferError;
 PyAPI_DATA(PyObject *) PyExc_EOFError;
 PyAPI_DATA(PyObject *) PyExc_FloatingPointError;
 PyAPI_DATA(PyObject *) PyExc_OSError;
+PyAPI_DATA(PyObject *) PyExc_ImportCycleError;
 PyAPI_DATA(PyObject *) PyExc_ImportError;
 #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03060000
 PyAPI_DATA(PyObject *) PyExc_ModuleNotFoundError;
diff --git a/Lib/_compat_pickle.py b/Lib/_compat_pickle.py
@@ -244,6 +244,7 @@
     REVERSE_NAME_MAPPING[('builtins', excname)] = ('exceptions', 'OSError')
 
 PYTHON3_IMPORTERROR_EXCEPTIONS = (
+    'ImportCycleError',
     'ModuleNotFoundError',
 )
 
diff --git a/Lib/test/exception_hierarchy.txt b/Lib/test/exception_hierarchy.txt
@@ -14,6 +14,7 @@ BaseException
       +-- BufferError
       +-- EOFError
       +-- ImportError
+      |    +-- ImportCycleError
       |    +-- ModuleNotFoundError
       +-- LookupError
       |    +-- IndexError
diff --git a/Lib/test/lazyimports/cycle_with_load_global/__init__.py b/Lib/test/lazyimports/cycle_with_load_global/__init__.py
@@ -0,0 +1,7 @@
+try:
+    from . import a
+    a
+# we expect a circular import ImportError; anything else is wrong
+except ImportError as e:
+    if "circular import" not in str(e):
+        raise
diff --git a/Lib/test/lazyimports/cycle_with_load_global/a.py b/Lib/test/lazyimports/cycle_with_load_global/a.py
@@ -0,0 +1,6 @@
+from .b import B
+
+def get_B():
+    return B
+
+B
diff --git a/Lib/test/lazyimports/cycle_with_load_global/b.py b/Lib/test/lazyimports/cycle_with_load_global/b.py
@@ -0,0 +1,5 @@
+B = "B"
+
+from .a import get_B
+
+get_B()
diff --git a/Lib/test/test_compiler/test_strict/test_loader.py b/Lib/test/test_compiler/test_strict/test_loader.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import _imp
 import dis
 import gc
 import io
@@ -2568,3 +2569,34 @@ def f(c: mod.C):
             # version of the C class, but the argument check will be against the
             # old version of the C class from the failed import
             self.assertIs(other.f(c), c)
+
+    def test_strict_lazy_import_cycle(self):
+        self.sbx.write_file(
+            "mod/__init__.py",
+            """
+            import __strict__
+            from . import version
+            """,
+        )
+        self.sbx.write_file(
+            "mod/version.py",
+            """
+            import __strict__
+            VERSION = 1
+            """,
+        )
+        self.sbx.write_file(
+            "entry.py",
+            """
+            from mod import version
+            v = version.VERSION
+            """,
+        )
+        for lazy in [True, False]:
+            with self.subTest(lazy=lazy):
+                orig = _imp._set_lazy_imports(lazy)
+                try:
+                    mod = self.sbx.strict_import("entry")
+                finally:
+                    _imp._set_lazy_imports(*orig)
+                self.assertEqual(mod.v, 1)
diff --git a/Objects/exceptions.c b/Objects/exceptions.c
@@ -779,6 +779,14 @@ ComplexExtendsException(PyExc_Exception, ImportError,
                         "Import can't find module, or can't find name in "
                         "module.");
 
+/*
+ *    ImportCycleError extends ImportError
+ */
+
+
+MiddlingExtendsException(PyExc_ImportError, ImportCycleError, ImportError,
+                         "Import produces a cycle.");
+
 /*
  *    ModuleNotFoundError extends ImportError
  */
@@ -2686,6 +2694,7 @@ _PyExc_Init(PyInterpreterState *interp)
     PRE_INIT(SystemExit);
     PRE_INIT(KeyboardInterrupt);
     PRE_INIT(ImportError);
+    PRE_INIT(ImportCycleError);
     PRE_INIT(ModuleNotFoundError);
     PRE_INIT(OSError);
     PRE_INIT(EOFError);
@@ -2821,6 +2830,7 @@ _PyBuiltins_AddExceptions(PyObject *bltinmod)
     POST_INIT(SystemExit);
     POST_INIT(KeyboardInterrupt);
     POST_INIT(ImportError);
+    POST_INIT(ImportCycleError);
     POST_INIT(ModuleNotFoundError);
     POST_INIT(OSError);
     INIT_ALIAS(EnvironmentError, OSError);
diff --git a/Objects/moduleobject.c b/Objects/moduleobject.c
@@ -760,6 +760,9 @@ Ci_module_lookupattro_impl(PyModuleObject *m, PyObject *name, int suppress)
     }
     if (suppress) {
         if (PyErr_Occurred()) {
+            if (PyErr_ExceptionMatches(PyExc_ImportCycleError)) {
+                PyErr_Clear();
+            }
             return NULL;
         }
     }
@@ -1389,7 +1392,7 @@ static PyObject * strictmodule_patch_delete(PyObject *self, PyObject *args)
 
 
 static PyObject *
-strictmodule_lookupattro(PyStrictModuleObject *m, PyObject *name, int suppress)
+Ci_strictmodule_lookupattro_impl(PyStrictModuleObject *m, PyObject *name, int suppress)
 {
     PyObject *attr;
     if (Py_TYPE(m) != &PyStrictModule_Type || !PyUnicode_Check(name)) {
@@ -1426,7 +1429,8 @@ strictmodule_lookupattro(PyStrictModuleObject *m, PyObject *name, int suppress)
                     Py_INCREF(attr);
                     return attr;
                 } else if (PyErr_Occurred()) {
-                    if (suppress && PyErr_ExceptionMatches(PyExc_AttributeError)) {
+                    if (suppress && (PyErr_ExceptionMatches(PyExc_AttributeError) ||
+                                     PyErr_ExceptionMatches(PyExc_ImportCycleError))) {
                         PyErr_Clear();
                     }
                     return NULL;
@@ -1503,7 +1507,13 @@ strictmodule_lookupattro(PyStrictModuleObject *m, PyObject *name, int suppress)
 static PyObject *
 strictmodule_getattro(PyStrictModuleObject *m, PyObject *name)
 {
-    return strictmodule_lookupattro(m, name, 0);
+    return Ci_strictmodule_lookupattro_impl(m, name, 0);
+}
+
+PyObject*
+Ci_strictmodule_lookupattro(PyObject *m, PyObject *name, int suppress)
+{
+    return Ci_strictmodule_lookupattro_impl((PyStrictModuleObject*)m, name, 1);
 }
 
 static int
diff --git a/Objects/object.c b/Objects/object.c
@@ -1004,6 +1004,18 @@ _PyObject_LookupAttr(PyObject *v, PyObject *name, PyObject **result)
         }
         return 0;
     }
+#ifdef ENABLE_CINDERX
+    if (tp->tp_getattro == PyStrictModule_Type.tp_getattro) {
+        *result = Ci_strictmodule_lookupattro(v, name, 1);
+        if (*result != NULL) {
+            return 1;
+        }
+        if (PyErr_Occurred()) {
+            return -1;
+        }
+        return 0;
+    }
+#endif
     if (tp->tp_getattro != NULL) {
         *result = (*tp->tp_getattro)(v, name);
     }
diff --git a/Python/import.c b/Python/import.c
@@ -2079,6 +2079,51 @@ _PyImport_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name)
     return NULL;
 }
 
+static long
+_lazy_state_add(PyThreadState *tstate, PyObject *state_dict, PyObject *lazy_import, long num)
+ {
+    long long_value = 0;
+    PyObject *exc_type, *exc_value, *exc_tb;
+    PyErr_Fetch(&exc_type, &exc_value, &exc_tb);
+
+    PyObject *value = PyDict_GetItem(state_dict, lazy_import);
+    if (value != NULL) {
+        long_value = PyLong_AsLong(value);
+        if (long_value == -1 && _PyErr_Occurred(tstate)) {
+            goto error;
+        }
+    }
+    long_value += num;
+    assert(long_value >= 0);
+    if (long_value == 0) {
+        if (value != NULL) {
+            if (PyDict_DelItem(state_dict, lazy_import) < 0) {
+                long_value = -1;
+                goto error;
+            }
+        }
+    } else {
+        PyObject *new_value = PyLong_FromLong(long_value);
+        if (new_value == NULL) {
+            long_value = -1;
+            goto error;
+        }
+        if (PyDict_SetItem(state_dict, lazy_import, new_value) < 0) {
+            Py_DECREF(new_value);
+            long_value = -1;
+            goto error;
+        }
+        Py_DECREF(new_value);
+    }
+
+  error:
+    PyErr_Restore(exc_type, exc_value, exc_tb);
+    if (long_value == -1 && !_PyErr_Occurred(tstate)) {
+        PyErr_SetString(PyExc_RuntimeError, "lazy state not added correctly");
+    }
+    return long_value;
+}
+
 PyObject *
 _PyImport_LoadLazyImportTstate(PyThreadState *tstate, PyObject *lazy_import, int full)
 {
@@ -2091,6 +2136,21 @@ _PyImport_LoadLazyImportTstate(PyThreadState *tstate, PyObject *lazy_import, int
 
     PyLazyImportObject *lz = (PyLazyImportObject *)lazy_import;
 
+    long lvalue = _lazy_state_add(tstate, state_dict, lazy_import, 1);
+    if (lvalue < 0) {
+        return NULL;
+    }
+    if (lvalue > 10) {
+        PyObject *name = _PyLazyImport_GetName(lazy_import);
+        PyObject *errmsg = PyUnicode_FromFormat("cannot import name %R "
+                                                "(most likely due to a circular import)",
+                                                name);
+        PyErr_SetImportErrorSubclass(PyExc_ImportCycleError, errmsg, lz->lz_from, NULL);
+        Py_XDECREF(errmsg);
+        Py_XDECREF(name);
+        goto error;
+    }
+
     Py_ssize_t dot = -1;
     if (!full && lz->lz_attr != NULL) {
         full = 1;
@@ -2109,29 +2169,21 @@ _PyImport_LoadLazyImportTstate(PyThreadState *tstate, PyObject *lazy_import, int
                 goto error;
             }
             Py_INCREF(lz->lz_attr);
-            if (PyTuple_SetItem(fromlist, 0, lz->lz_attr) < 0) {
-                goto error;
-            }
+            PyTuple_SET_ITEM(fromlist, 0, lz->lz_attr);
         } else {
             Py_INCREF(lz->lz_attr);
             fromlist = lz->lz_attr;
         }
     }
     PyObject *globals = _PyEval_GetGlobals(tstate);
-    PyObject *zero = _PyLong_GetZero(); // borrowed reference
-
-    if (PyDict_Contains(state_dict, lazy_import)) {
-        goto error;
-    }
-    PyDict_SetItem(state_dict, lazy_import, _PyLong_GetOne());
 
     if (full) {
         obj = _PyImport_ImportName(lz->lz_builtins,
                                    globals,
                                    globals,
                                    lz->lz_from,
                                    fromlist,
-                                   zero);
+                                   _PyLong_GetZero());
     } else {
         PyObject *name = PyUnicode_Substring(lz->lz_from, 0, dot);
         if (name == NULL) {
@@ -2142,7 +2194,7 @@ _PyImport_LoadLazyImportTstate(PyThreadState *tstate, PyObject *lazy_import, int
                                    globals,
                                    name,
                                    fromlist,
-                                   zero);
+                                   _PyLong_GetZero());
         Py_DECREF(name);
     }
 
@@ -2159,25 +2211,27 @@ _PyImport_LoadLazyImportTstate(PyThreadState *tstate, PyObject *lazy_import, int
         }
     }
 
-    if (PyDict_DelItem(state_dict, lazy_import) < 0) {
-        Py_XDECREF(obj);
-        obj = NULL;
-        goto error;
-    }
-
-    if (obj != NULL) {
-        if (PyLazyImport_CheckExact(obj)) {
-            PyObject *value = _PyImport_LoadLazyImportTstate(tstate, obj, 0);
-            Py_DECREF(obj);
-            if (value == NULL) {
-                goto error;
-            }
-            obj = value;
+    if (PyLazyImport_CheckExact(obj)) {
+        PyObject *new_obj = _PyImport_LoadLazyImportTstate(tstate, obj, 0);
+        if (new_obj == NULL) {
+            goto error;
         }
-        assert(!PyLazyImport_CheckExact(obj));
+        Py_DECREF(obj);
+        obj = new_obj;
     }
+    assert(!PyLazyImport_CheckExact(obj));
+
+    goto ok;
 
   error:
+    Py_XDECREF(obj);
+    obj = NULL;
+
+  ok:
+    if (_lazy_state_add(tstate, state_dict, lazy_import, -1) < 0) {
+        Py_XDECREF(obj);
+        obj = NULL;
+    }
     Py_XDECREF(fromlist);
     return obj;
 }

Original file line number	Diff line number	Diff line change
`@@ -244,6 +244,7 @@`
`244`	`244`	`REVERSE_NAME_MAPPING[('builtins', excname)] = ('exceptions', 'OSError')`
`245`	`245`
`246`	`246`	`PYTHON3_IMPORTERROR_EXCEPTIONS = (`
	`247`	`+ 'ImportCycleError',`
`247`	`248`	`'ModuleNotFoundError',`
`248`	`249`	`)`
`249`	`250`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +B = "B"
++
 +from .a import get_B
++
 +get_B()