Override inherited overloads a direct base implements only in part

src/cpyrt/Dispatcher.cxx

@@ -11,14 +11,17 @@ using namespace cppjit;
 #include "cppjit_interop.h"
 
 // Standard
+#include <map>
 #include <set>
 #include <sstream>
 
 //----------------------------------------------------------------------------
 static inline void InjectMethod(interop::TCppMethod_t method,
                                 const std::string& mtCppName,
+                                const std::string& mtPyName,
                                 std::ostringstream& code) {
-  // inject implementation for an overridden method
+  // inject implementation for an overridden method, forwarding to the Python
+  // method of the given name (the C++ name, or its mapping for operators)
   using namespace cppjit::cpyrt;
 
   // method declaration
@@ -65,7 +68,7 @@ static inline void InjectMethod(interop::TCppMethod_t method,
   Utility::ConstructCallbackPreamble(retType, argtypes, code);
 
   // perform actual method call
-  code << "    PyObject* mtPyName = PyUnicode_FromString(\"" << mtCppName
+  code << "    PyObject* mtPyName = PyUnicode_FromString(\"" << mtPyName
        << "\");\n"
           "    PyObject* pyresult = PyObject_CallMethodObjArgs(iself, mtPyName";
   for (interop::TCppIndex_t i = 0; i < nArgs; ++i)
@@ -163,21 +166,61 @@ namespace {
 
 using namespace cppjit::interop;
 
-static inline std::vector<TCppMethod_t>
-FindBaseMethod(TCppScope_t tbase, const std::string mtCppName) {
-  // Recursively walk the inheritance tree to find the overloads of the named
-  // method
-  std::vector<TCppMethod_t> result;
-  result = GetMethodsFromName(tbase, mtCppName);
-  if (result.empty()) {
-    for (TCppIndex_t ibase = 0; ibase < GetNumBases(tbase); ++ibase) {
-      TCppScope_t b = interop::GetBaseScope(tbase, ibase);
-      result = FindBaseMethod(b, mtCppName);
-      if (!result.empty())
-        break;
-    }
+// signatures of the methods already overridden, per C++ method name
+typedef std::map<std::string, std::set<std::string>> Overridden_t;
+
+static inline std::string GetOverrideSignature(TCppMethod_t method) {
+  // What an override has to match: the canonical argument types (ignoring
+  // top-level cv-qualifiers, which are not part of the function type) and
+  // constness; unlike interop::GetMethodSignature, which spells the argument
+  // types as written, this is the same string for all declarations of one
+  // signature, so it can be used to tell overloads apart
+  std::string sig = "(";
+  TCppIndex_t nArgs = GetMethodNumArgs(method);
+  for (TCppIndex_t i = 0; i < nArgs; ++i) {
+    if (i != 0)
+      sig += ", ";
+    sig += GetMethodArgCanonUnqualTypeAsString(method, i);
+  }
+  sig += IsConstMethod(method) ? ") const" : ")";
+  return sig;
+}
+
+static inline std::string GetPyMethodName(TCppMethod_t method,
+                                          const std::string& mtCppName) {
+  // The name under which the Python class overrides the method: operators map
+  // onto their Python special methods (e.g. operator() onto __call__); the
+  // result is empty for operators that have no Python side (e.g. operator new)
+  return cpyrt::Utility::MapOperatorName(mtCppName,
+                                         GetMethodNumArgs(method) != 0);
+}
+
+static void InjectInheritedOverloads(TCppScope_t tbase,
+                                     const std::set<std::string>& names,
+                                     Overridden_t& overridden,
+                                     std::ostringstream& code) {
+  // Walk the inheritance tree for virtual overloads of the named methods that
+  // no more derived class declares: a base may implement only some overloads
+  // of a method the Python class overrides, leaving the others (e.g. pure
+  // virtual ones) to be overridden here, or the dispatcher class remains
+  // abstract. Non-virtual methods are not dispatched to Python either way, so
+  // only the signatures of the virtual ones are recorded.
+  std::vector<TCppMethod_t> methods;
+  GetClassMethods(tbase, methods);
+  for (const auto& method : methods) {
+    if (!IsVirtualMethod(method))
+      continue;
+    const std::string& mtCppName = GetName(TCppScope_t(method.data));
+    std::string mtPyName = GetPyMethodName(method, mtCppName);
+    if (mtPyName.empty() || names.find(mtPyName) == names.end())
+      continue;
+    if (overridden[mtCppName].insert(GetOverrideSignature(method)).second)
+      InjectMethod(method, mtCppName, mtPyName, code);
   }
-  return result;
+
+  for (TCppIndex_t ibase = 0; ibase < GetNumBases(tbase); ++ibase)
+    InjectInheritedOverloads(GetBaseScope(tbase, ibase), names, overridden,
+                             code);
 }
 
 } // unnamed namespace
@@ -289,18 +332,22 @@ bool cpyrt::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
     code << "  virtual ~" << derivedName << "() {}\n";
 
   // methods: first collect all callables, then get overrides from base classes,
-  // for those that are still missing, search the hierarchy
-  PyObject* clbs = PyDict_New();
+  // then search the rest of the hierarchy for overloads these do not cover
+  std::set<std::string> clbs;
   PyObject* items = PyDict_Items(dct);
   for (Py_ssize_t i = 0; i < PyList_GET_SIZE(items); ++i) {
+    PyObject* key = PyTuple_GET_ITEM(PyList_GET_ITEM(items, i), 0);
     PyObject* value = PyTuple_GET_ITEM(PyList_GET_ITEM(items, i), 1);
-    if (PyCallable_Check(value))
-      PyDict_SetItem(clbs, PyTuple_GET_ITEM(PyList_GET_ITEM(items, i), 0),
-                     value);
+    if (PyUnicode_Check(key) && PyCallable_Check(value)) {
+      if (const char* name = PyUnicode_AsUTF8(key))
+        clbs.insert(name);
+      else
+        PyErr_Clear();
+    }
   }
   Py_DECREF(items);
-  if (PyDict_DelItem(clbs, PyStrings::gInit) != 0)
-    PyErr_Clear();
+  clbs.erase("__init__");
+  Overridden_t overridden;
 
   // protected methods and data need their access changed in the C++ trampoline
   // and then exposed on the Python side; so, collect their names as we go along
@@ -339,13 +386,8 @@ bool cpyrt::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
 
       std::string mtCppName =
           interop::GetName(interop::TCppScope_t(method.data));
-      PyObject* key = PyUnicode_FromString(mtCppName.c_str());
-      int contains = PyDict_Contains(dct, key);
-      if (contains == -1)
-        PyErr_Clear();
-      if (contains != 1) {
-        Py_DECREF(key);
-
+      std::string mtPyName = GetPyMethodName(method, mtCppName);
+      if (mtPyName.empty() || clbs.find(mtPyName) == clbs.end()) {
         // if the method is protected, we expose it through re-declaration and
         // forwarding (using does not work here b/c there may be private
         // overloads)
@@ -380,11 +422,8 @@ bool cpyrt::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
         continue;
       }
 
-      InjectMethod(method, mtCppName, code);
-
-      if (PyDict_DelItem(clbs, key) != 0)
-        PyErr_Clear(); // happens for overloads
-      Py_DECREF(key);
+      if (overridden[mtCppName].insert(GetOverrideSignature(method)).second)
+        InjectMethod(method, mtCppName, mtPyName, code);
     }
 
     // support for templated ctors in single inheritance (TODO: also multi
@@ -410,33 +449,16 @@ bool cpyrt::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
       has_ctors += 1;
   }
 
-  // try to locate left-overs in base classes
-  for (const auto& binfo : base_infos) {
-    if (PyDict_Size(clbs)) {
+  // overloads further up the hierarchy, whether of a method the direct bases
+  // do not declare at all or of one they implement only in part
+  if (!clbs.empty()) {
+    for (const auto& binfo : base_infos) {
       size_t nbases = interop::GetNumBases(binfo.btype);
-      for (size_t ibase = 0; ibase < nbases; ++ibase) {
-        interop::TCppScope_t tbase =
-            (interop::TCppScope_t)interop::GetBaseScope(binfo.btype, ibase);
-
-        PyObject* keys = PyDict_Keys(clbs);
-        for (Py_ssize_t i = 0; i < PyList_GET_SIZE(keys); ++i) {
-          // TODO: should probably invert this looping; but that makes handling
-          // overloads clunky
-          PyObject* key = PyList_GET_ITEM(keys, i);
-          std::string mtCppName = PyUnicode_AsUTF8(key);
-          const auto& methods = FindBaseMethod(tbase, mtCppName);
-          for (auto method : methods)
-            InjectMethod(method, mtCppName, code);
-          if (!methods.empty()) {
-            if (PyDict_DelItem(clbs, key) != 0)
-              PyErr_Clear();
-          }
-        }
-        Py_DECREF(keys);
-      }
+      for (size_t ibase = 0; ibase < nbases; ++ibase)
+        InjectInheritedOverloads(interop::GetBaseScope(binfo.btype, ibase),
+                                 clbs, overridden, code);
     }
   }
-  Py_DECREF(clbs);
 
   // constructors: build up from the argument types of the base class, for use
   // by the Python derived class (inheriting with/ "using" does not work b/c

src/interop/cppjit_interop.h

@@ -278,6 +278,11 @@ std::string GetMethodArgTypeAsString(TCppMethod_t method, TCppIndex_t iarg);
 RPY_EXPORTED
 std::string GetMethodArgCanonTypeAsString(TCppMethod_t method,
                                           TCppIndex_t iarg);
+// canonical type with top-level cv-qualifiers removed, as it figures in the
+// function type (`f(const int)` and `f(int)` are the same overload)
+RPY_EXPORTED
+std::string GetMethodArgCanonUnqualTypeAsString(TCppMethod_t method,
+                                                TCppIndex_t iarg);
 RPY_EXPORTED
 std::string GetMethodArgDefault(TCppMethod_t, TCppIndex_t iarg);
 RPY_EXPORTED
@@ -297,6 +302,8 @@ RPY_EXPORTED
 std::string GetDoxygenComment(TCppScope_t scope, bool strip_markers = true);
 RPY_EXPORTED
 bool IsConstMethod(TCppMethod_t);
+RPY_EXPORTED
+bool IsVirtualMethod(TCppMethod_t);
 // Templated method/function reflection information
 // ------------------------------------
 RPY_EXPORTED

src/interop/interop_wrapper.cxx

@@ -1294,6 +1294,14 @@ std::string interop::GetMethodArgCanonTypeAsString(TCppMethod_t method,
       Cpp::GetCanonicalType(Cpp::GetFunctionArgType(method, iarg)));
 }
 
+std::string interop::GetMethodArgCanonUnqualTypeAsString(TCppMethod_t method,
+                                                         TCppIndex_t iarg) {
+  std::lock_guard<std::recursive_mutex> Lock(InterOpMutex);
+  return Cpp::GetTypeAsString(Cpp::RemoveTypeQualifier(
+      Cpp::GetCanonicalType(Cpp::GetFunctionArgType(method, iarg)),
+      Cpp::QualKind::Const | Cpp::QualKind::Volatile));
+}
+
 std::string interop::GetMethodArgDefault(TCppMethod_t method,
                                          TCppIndex_t iarg) {
   if (!method)
@@ -1415,6 +1423,13 @@ bool interop::IsConstMethod(TCppMethod_t method) {
   return Cpp::IsConstMethod(method);
 }
 
+bool interop::IsVirtualMethod(TCppMethod_t method) {
+  if (!method)
+    return false;
+  std::lock_guard<std::recursive_mutex> Lock(InterOpMutex);
+  return Cpp::IsVirtualMethod(method);
+}
+
 void interop::GetTemplatedMethods(TCppScope_t scope,
                                   std::vector<interop::TCppMethod_t>& methods) {
   std::lock_guard<std::recursive_mutex> Lock(InterOpMutex);

test/test_crossinheritance.py

@@ -1918,3 +1918,94 @@ class MyUIntBaseClass {
         class MyUIntDerivedClass(cppjit.gbl.MyUIntBaseClass):
             def give_unsigned_int(self):
                 return 1
+
+    def test40_override_overloads_split_across_hierarchy(self):
+        """Override pure virtual overloads an intermediate base left out"""
+
+        import cppjit
+
+        cppjit.cppdef("""namespace SplitOverloads {
+        struct Impl {
+          virtual ~Impl() {}
+          virtual int operator()(int*) = 0;
+          virtual int operator()() = 0;
+          virtual int run(int) = 0;
+          virtual int run() const = 0;
+        };
+        struct Standalone : Impl {
+          int operator()(int*) override { return -1; }
+          int run(int) override { return -2; }
+        };
+        int call_ptr(Impl& i) { int x = 5; return i(&x); }
+        int call_void(Impl& i) { return i(); }
+        int run_int(Impl& i) { return i.run(9); }
+        int run_const(const Impl& i) { return i.run(); } }""")
+
+        ns = cppjit.gbl.SplitOverloads
+
+        # the dispatcher used to override only the overloads Standalone itself
+        # declares, so the pure virtual ones inherited from Impl kept it abstract
+        class PyStandalone(ns.Standalone):
+            def __call__(self, *args):
+                return 100 + len(args)
+
+            def run(self, *args):
+                return 200 + len(args)
+
+        obj = PyStandalone()
+        assert ns.call_ptr(obj) == 101
+        assert ns.call_void(obj) == 100
+        assert ns.run_int(obj) == 201
+        assert ns.run_const(obj) == 200
+
+        # overriding only some of the overloads still leaves the class abstract
+        with raises(TypeError, match="abstract"):
+            type("Partial", (ns.Standalone,), {"run": lambda self, *a: 0})()
+
+    def test41_override_same_signature_declared_twice(self):
+        """Override a signature several classes in the hierarchy declare"""
+
+        import cppjit
+
+        cppjit.cppdef("""namespace TwiceDeclared {
+        struct Grand {
+          virtual ~Grand() {}
+          virtual int f(int) { return 7; }
+          virtual int g(int*) = 0;
+          virtual int g() = 0;
+        };
+        // same overloads, with top-level const that is not part of the type
+        struct Parent : Grand {
+          int f(const int) override { return 1; }
+          int g(int* const) override { return 2; }
+        };
+        int call_f(Grand& o) { return o.f(1); }
+        int call_g(Grand& o) { int x = 5; return o.g(&x) + o.g(); }
+
+        struct A { virtual ~A() {} virtual int h() = 0; };
+        struct B { virtual ~B() {} virtual int h() = 0; };
+        int call_a(A& a) { return a.h(); }
+        int call_b(B& b) { return b.h(); } }""")
+
+        ns = cppjit.gbl.TwiceDeclared
+
+        # the overloads in Grand and Parent must each be overridden only once
+        class PyParent(ns.Parent):
+            def f(self, x):
+                return 100
+
+            def g(self, *args):
+                return 200 + len(args)
+
+        obj = PyParent()
+        assert ns.call_f(obj) == 100
+        assert ns.call_g(obj) == 401
+
+        # same for a pure virtual shared by two bases in multiple inheritance
+        class PyAB(cppjit.multi(ns.A, ns.B)):
+            def h(self):
+                return 300
+
+        obj = PyAB()
+        assert ns.call_a(obj) == 300
+        assert ns.call_b(obj) == 300