From 817bfe54e26cdd71ee1de69fa45c305362f360d1 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 14 Oct 2026 08:53:50 +0000 Subject: [PATCH] Override inherited overloads a direct base implements only in part The dispatcher matched the methods a Python class overrides against the methods its direct C++ base declares itself, and only looked further up the hierarchy for names the base does not declare at all. A base that implements some overloads of a method and inherits others as pure virtual thus had only its own overloads overridden, and the dispatcher class remained abstract. Walk the whole hierarchy for every overridden name and override each virtual overload not already covered by a more derived class, keyed by canonical argument types and constness. The same key also prevents a signature from being defined twice when two bases declare it. Operators are matched by their Python name, so that e.g. defining __call__ overrides a virtual operator(). Fixes #146 --- src/cpyrt/Dispatcher.cxx | 140 ++++++++++++++++++-------------- src/interop/cppjit_interop.h | 7 ++ src/interop/interop_wrapper.cxx | 15 ++++ test/test_crossinheritance.py | 91 +++++++++++++++++++++ 4 files changed, 194 insertions(+), 59 deletions(-) diff --git a/src/cpyrt/Dispatcher.cxx b/src/cpyrt/Dispatcher.cxx index 40b1dbdf..b36cd2eb 100644 --- a/src/cpyrt/Dispatcher.cxx +++ b/src/cpyrt/Dispatcher.cxx @@ -11,14 +11,17 @@ using namespace cppjit; #include "cppjit_interop.h" // Standard +#include #include #include //---------------------------------------------------------------------------- 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 -FindBaseMethod(TCppScope_t tbase, const std::string mtCppName) { - // Recursively walk the inheritance tree to find the overloads of the named - // method - std::vector 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> 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& 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 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 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 diff --git a/src/interop/cppjit_interop.h b/src/interop/cppjit_interop.h index 59c1a95b..5eba3b77 100644 --- a/src/interop/cppjit_interop.h +++ b/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 diff --git a/src/interop/interop_wrapper.cxx b/src/interop/interop_wrapper.cxx index d26697c8..74ad16ca 100644 --- a/src/interop/interop_wrapper.cxx +++ b/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 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 Lock(InterOpMutex); + return Cpp::IsVirtualMethod(method); +} + void interop::GetTemplatedMethods(TCppScope_t scope, std::vector& methods) { std::lock_guard Lock(InterOpMutex); diff --git a/test/test_crossinheritance.py b/test/test_crossinheritance.py index 2e537df9..b2025c84 100644 --- a/test/test_crossinheritance.py +++ b/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