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