[cpyrt] Stop the using-directive search at a cycle

src/cpyrt/CPPScope.cxx

@@ -320,7 +320,18 @@ static PyObject* pt_new(PyTypeObject* subtype, PyObject* args, PyObject* kwds) {
 }
 
 //----------------------------------------------------------------------------
+// The (scope, name) pairs that one lookup has searched through using
+// directives ([namespace.qual]/3), so that using-directive cycles terminate.
+using UsingSearch = std::set<std::pair<PyObject*, std::string>>;
+static thread_local UsingSearch* tUsingSearch = nullptr;
+// Set by the using-directive loop for the scope lookup that it starts next.
+static thread_local bool tUsingHop = false;
+
 static PyObject* meta_getattro(PyObject* pyclass, PyObject* pyname) {
+  // read before the type lookup below can run Python code
+  const bool using_hop = tUsingHop;
+  tUsingHop = false;
+
   // normal type-based lookup
   PyObject* attr = PyType_Type.tp_getattro(pyclass, pyname);
   if (pyclass == (PyObject*)&CPPInstance_Type)
@@ -505,19 +516,30 @@ static PyObject* meta_getattro(PyObject* pyclass, PyObject* pyname) {
       }
     }
 
-    // try all outstanding using namespaces in turn to find the attribute (will
-    // cache locally later; TODO: doing so may cause pathological cases)
-    for (auto pyref : *klass->fImp.fUsing) {
-      PyObject* pyuscope = cpyrt_GetWeakRef(pyref);
-      if (pyuscope) {
-        attr = PyObject_GetAttr(pyuscope, pyname);
-        if (!attr)
-          PyErr_Clear();
-        Py_DECREF(pyuscope);
+    // A hop from the loop below continues its lookup's search; any other
+    // lookup, e.g. from Python code that runs during the loop, starts afresh.
+    UsingSearch fresh;
+    UsingSearch* const outer = tUsingSearch;
+    if (!using_hop || !outer)
+      tUsingSearch = &fresh;
+    if (tUsingSearch->emplace(pyclass, name).second) {
+      // try all outstanding using namespaces in turn to find the attribute
+      // (will cache locally later; TODO: doing so may cause pathological cases)
+      for (auto pyref : *klass->fImp.fUsing) {
+        PyObject* pyuscope = cpyrt_GetWeakRef(pyref);
+        if (pyuscope) {
+          tUsingHop = true;
+          attr = PyObject_GetAttr(pyuscope, pyname);
+          tUsingHop = false;
+          if (!attr)
+            PyErr_Clear();
+          Py_DECREF(pyuscope);
+        }
+        if (attr)
+          break;
       }
-      if (attr)
-        break;
     }
+    tUsingSearch = outer;
   }
 
   // if the attribute was not found but could possibly have been shadowed,

test/test_advancedcpp.py

@@ -1098,3 +1098,85 @@ def test31_virtual_base_downcast(self):
         assert isinstance(obj3, ns.Visible3)
         assert obj3.value == 23
         assert obj3.z == 7
+
+    def test32_using_directive_cycle(self):
+        """Lookup through cyclic using directives searches each namespace once"""
+
+        import cppjit
+
+        cppjit.cppdef("""\
+        namespace udir_cycle_a {}
+        namespace udir_cycle_p { using namespace udir_cycle_a; int x = 1; }
+        namespace udir_cycle_a { using namespace udir_cycle_p; int y = 2; }""")
+
+        a = cppjit.gbl.udir_cycle_a
+        p = cppjit.gbl.udir_cycle_p
+        assert a.x == 1
+        assert p.y == 2
+
+        with raises(AttributeError):
+            a.nothere
+        with raises(AttributeError):
+            p.nothere
+
+    def test33_using_directive_lookup_from_other_thread(self):
+        """A lookup from another thread does not see a running lookup's search"""
+
+        import threading
+
+        import cppjit
+
+        cppjit.cppdef("""\
+        namespace udir_thread_a {}
+        namespace udir_thread_p { using namespace udir_thread_a; }
+        namespace udir_thread_a { using namespace udir_thread_p; }""")
+
+        a = cppjit.gbl.udir_thread_a
+        p = cppjit.gbl.udir_thread_p
+        started, inner = [], []
+
+        def lookup():
+            try:
+                inner.append(a.hook)
+            except AttributeError:
+                inner.append(None)
+
+        class Hook:
+            # udir_thread_a.hook resolves only through the using directive, so
+            # __get__ runs while that lookup's search is still open
+            def __get__(self, obj, owner):
+                if not started:
+                    started.append(True)
+                    t = threading.Thread(target=lookup)
+                    t.start()
+                    t.join()  # releases the GIL
+                return 42
+
+        p.hook = Hook()
+        assert a.hook == 42
+        assert inner == [42]
+
+    def test34_using_directive_lookup_reentered(self):
+        """A lookup from Python code that runs during a lookup searches afresh"""
+
+        import cppjit
+
+        cppjit.cppdef("""\
+        namespace udir_reenter_a {}
+        namespace udir_reenter_p { using namespace udir_reenter_a; }
+        namespace udir_reenter_a { using namespace udir_reenter_p; }""")
+
+        a = cppjit.gbl.udir_reenter_a
+        p = cppjit.gbl.udir_reenter_p
+        inner = []
+
+        class Hook:
+            def __get__(self, obj, owner):
+                if not inner:
+                    inner.append(None)
+                    inner[0] = a.hook
+                return 42
+
+        p.hook = Hook()
+        assert a.hook == 42
+        assert inner == [42]