From 6aea8b0d36ca039509be9dc6e9dfdf0a2173c034 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 9 Oct 2026 17:48:38 +0000 Subject: [PATCH] Take function pointer types from the AST instead of parsing type strings The function pointer branches of CreateExecutor() and CreateConverter() sliced the printed type string (e.g. "int (*)(double, char)") to recover the return type and the argument list, as flagged by a "find better way of finding the type" TODO. Replace this with a new interop::GetFnTypeSigAsString(), built on GetFnTypeSig(), which asks clang for the pointee function prototype's return and parameter types. GetFnTypeSig() now guards against non-prototype types, as Cpp::GetFnTypeSignature() crashes on those. This fixes two bugs of the string parsing: - the type-based executor path removed all spaces before slicing, so any multi-word return type such as `unsigned int (*)()` broke with an attempt to compile `std::function`; - member function pointers matched the "::*)" check and produced a bogus std::function. They are not function pointers and are now rejected (returning one yields None as before, a data member of that type is reported as unconvertible). The string-based overloads keep the type from the lookup that ResolveName() performed anyway, and the regex used by the string-based CreateConverter() is dropped. --- src/cpyrt/Converters.cxx | 40 ++++++++++------------------- src/cpyrt/Executors.cxx | 35 ++++++++++--------------- src/interop/cppjit_interop.h | 9 +++++++ src/interop/interop_wrapper.cxx | 28 ++++++++++++++++++++ test/test_datatypes.py | 16 ++++++++++++ test/test_regression.py | 45 +++++++++++++++++++++++++++++++++ 6 files changed, 125 insertions(+), 48 deletions(-) diff --git a/src/cpyrt/Converters.cxx b/src/cpyrt/Converters.cxx index 58a4aac2..08ce5af7 100644 --- a/src/cpyrt/Converters.cxx +++ b/src/cpyrt/Converters.cxx @@ -26,7 +26,6 @@ using namespace cppjit; #include #include #include // for wstring_convert -#include #include #include // for ptrdiff_t #include @@ -46,9 +45,6 @@ static ConvFactories_t gConvFactories; // special objects extern PyObject* gNullPtrObject; extern PyObject* gDefaultObject; - -// regular expression for matching function pointer -static std::regex s_fnptr("\\((\\w*:*)*\\*&*\\)"); } // namespace cppjit::cpyrt // Define our own PyUnstable_Object_IsUniqueReferencedTemporary function if the @@ -3454,8 +3450,11 @@ cppjit::cpyrt::CreateConverter(const std::string& fullType, cdims_t dims) { return (h->second)(dims); } - // resolve typedefs etc. - const std::string& resolvedType = interop::ResolveName(fullType); + // resolve typedefs etc. (the type itself is kept for function pointers) + interop::TCppType_t type = + interop::GetType(fullType, /*enable_slow_lookup=*/true); + const std::string resolvedType = + type ? interop::GetTypeAsString(interop::ResolveType(type)) : fullType; // a full, qualified matching converter is preferred if (resolvedType != fullType) { @@ -3615,16 +3614,10 @@ cppjit::cpyrt::CreateConverter(const std::string& fullType, cdims_t dims) { // -- Cling WORKAROUND result = selectInstanceCnv(klass, cpd, dims, isConst, control); } - } else { - std::smatch sm; - if (std::regex_search(resolvedType, sm, s_fnptr)) { - // this is a function pointer - auto pos1 = sm.position(0); - auto pos2 = resolvedType.rfind(')'); - result = new FunctionPointerConverter( - resolvedType.substr(0, pos1), - resolvedType.substr(pos1 + sm.length(), pos2 - 1)); - } + } else if (std::string fnRet, fnSig; + interop::GetFnTypeSigAsString(type, fnRet, fnSig)) { + // this is a function pointer + result = new FunctionPointerConverter(fnRet, fnSig); } const std::string failure_msg("Failed to convert type: " + fullType + "; resolved: " + resolvedType + @@ -3856,17 +3849,10 @@ cppjit::cpyrt::CreateConverter(interop::TCppType_t type, cdims_t dims) { // converters for known C++ classes and default (void*) Converter* result = nullptr; interop::TCppScope_t klass = interop::GetScopeFromType(realType); - if (resolvedTypeStr.find("(*)") != std::string::npos || - (resolvedTypeStr.find("::*)") != std::string::npos)) { - // this is a function function pointer - // TODO: find better way of finding the type - auto pos1 = resolvedTypeStr.find('('); - auto pos2 = resolvedTypeStr.find("*)"); - auto pos3 = resolvedTypeStr.rfind(')'); - std::string return_type = resolvedTypeStr.substr(0, pos1); - result = new FunctionPointerConverter( - return_type.erase(return_type.find_last_not_of(" ") + 1), - resolvedTypeStr.substr(pos2 + 2, pos3 - pos2 - 1)); + if (std::string fnRet, fnSig; + interop::GetFnTypeSigAsString(resolvedType, fnRet, fnSig)) { + // this is a function pointer + result = new FunctionPointerConverter(fnRet, fnSig); } else if ((realTypeStr != "std::byte") && (klass || (klass = interop::GetFullScope(realTypeStr)))) { // std::byte is a special enum class used to access raw memory diff --git a/src/cpyrt/Executors.cxx b/src/cpyrt/Executors.cxx index dc119961..68add1fe 100644 --- a/src/cpyrt/Executors.cxx +++ b/src/cpyrt/Executors.cxx @@ -876,8 +876,11 @@ cpyrt::Executor* cpyrt::CreateExecutor(const std::string& fullType, if (h != gExecFactories.end()) return (h->second)(dims); - // resolve typedefs etc. - const std::string resolvedType = interop::ResolveName(fullType); + // resolve typedefs etc. (the type itself is kept for function pointers) + interop::TCppType_t type = + interop::GetType(fullType, /*enable_slow_lookup=*/true); + const std::string resolvedType = + type ? interop::GetTypeAsString(interop::ResolveType(type)) : fullType; // a full, qualified matching executor is preferred if (resolvedType != fullType) { @@ -948,16 +951,10 @@ cpyrt::Executor* cpyrt::CreateExecutor(const std::string& fullType, result = new InstancePtrRefExecutor(klass); } else result = new InstancePtrExecutor(klass); - } else if (resolvedType.find("(*)") != std::string::npos || - (resolvedType.find("::*)") != std::string::npos)) { + } else if (std::string fnRet, fnSig; + interop::GetFnTypeSigAsString(type, fnRet, fnSig)) { // this is a function pointer - // TODO: find better way of finding the type - auto pos1 = resolvedType.find('('); - auto pos2 = resolvedType.find("*)"); - auto pos3 = resolvedType.rfind(')'); - result = new FunctionPointerExecutor( - resolvedType.substr(0, pos1), - resolvedType.substr(pos2 + 2, pos3 - pos2 - 1)); + result = new FunctionPointerExecutor(fnRet, fnSig); } else { // unknown: void* may work ("user knows best"), void will fail on use of // return value @@ -1090,16 +1087,12 @@ cpyrt::Executor* cpyrt::CreateExecutor(interop::TCppType_t type, cdims_t dims) { result = new InstancePtrRefExecutor(klass); } else result = new InstancePtrExecutor(klass); - } else if (realTypeStr.find("(*)") != std::string::npos || - (realTypeStr.find("::*)") != std::string::npos)) { - // this is a function pointer - // TODO: find better way of finding the type - auto pos1 = realTypeStr.find('('); - auto pos2 = realTypeStr.find("*)"); - auto pos3 = realTypeStr.rfind(')'); - result = new FunctionPointerExecutor( - realTypeStr.substr(0, pos1), - realTypeStr.substr(pos2 + 2, pos3 - pos2 - 1)); + } else if (std::string fnRet, fnSig; + interop::GetFnTypeSigAsString(resolvedType, fnRet, fnSig)) { + // this is a function pointer (checked on the resolved type: the real type + // of a member function pointer is a bare function type, which must not + // pass here, as it would return 16 bytes through an 8-byte return slot) + result = new FunctionPointerExecutor(fnRet, fnSig); } else { // unknown: void* may work ("user knows best"), void will fail on use of // return value diff --git a/src/interop/cppjit_interop.h b/src/interop/cppjit_interop.h index 59c1a95b..c07b7583 100644 --- a/src/interop/cppjit_interop.h +++ b/src/interop/cppjit_interop.h @@ -289,6 +289,15 @@ RPY_EXPORTED TCppType_t GetFnTypeFromStdFn(TCppType_t fn_type); RPY_EXPORTED void GetFnTypeSig(TCppType_t fn_type, std::vector& arg_types); +/// Spells out a function type, or a pointer or reference to one, as the +/// return type and the parenthesized argument list, e.g. "int" and +/// "(double, const char *)"; the argument list is formatted as by +/// GetMethodSignature(). Member function pointers are not function pointers +/// and are rejected. +/// \returns false if \p fn_type is not a function (pointer) type. +RPY_EXPORTED +bool GetFnTypeSigAsString(TCppType_t fn_type, std::string& ret_type, + std::string& signature); RPY_EXPORTED bool IsSameType(TCppType_t typ1, TCppType_t typ2); RPY_EXPORTED diff --git a/src/interop/interop_wrapper.cxx b/src/interop/interop_wrapper.cxx index d26697c8..b5242ad3 100644 --- a/src/interop/interop_wrapper.cxx +++ b/src/interop/interop_wrapper.cxx @@ -1378,13 +1378,41 @@ interop::TCppType_t interop::GetFnTypeFromStdFn(TCppType_t fn_type) { void interop::GetFnTypeSig(TCppType_t fn_type, std::vector& arg_types) { + if (!fn_type) + return; fn_type = Cpp::IsReferenceType(fn_type) ? Cpp::GetNonReferenceType(fn_type) : fn_type; fn_type = Cpp::IsPointerType(fn_type) ? Cpp::GetPointeeType(fn_type) : fn_type; + // Cpp::GetFnTypeSignature() requires a function prototype: anything else + // (e.g. a member function pointer, or a plain object type) yields nothing + if (!Cpp::IsFunctionProtoType(fn_type)) + return; Cpp::GetFnTypeSignature(fn_type, arg_types); } +bool interop::GetFnTypeSigAsString(TCppType_t fn_type, std::string& ret_type, + std::string& signature) { + // the first entry is the return type, followed by the argument types + std::vector types; + GetFnTypeSig(fn_type, types); + if (types.empty()) + return false; + + ret_type = GetTypeAsString(types[0]); + + std::ostringstream sig; + sig << "("; + for (std::size_t iarg = 1; iarg < types.size(); ++iarg) { + if (iarg != 1) + sig << ", "; + sig << GetTypeAsString(types[iarg]); + } + sig << ")"; + signature = sig.str(); + return true; +} + bool interop::IsSameType(TCppType_t typ1, TCppType_t typ2) { return Cpp::IsSameType(typ1, typ2); } diff --git a/test/test_datatypes.py b/test/test_datatypes.py index 96d63070..e6071fd4 100644 --- a/test/test_datatypes.py +++ b/test/test_datatypes.py @@ -2783,6 +2783,22 @@ def test55_qt_cache_alias_collision(self): ns.take_int8(101) raises(TypeError, ns.take_int8, "e") + def test56_function_pointer_return_multiword_types(self): + """A returned function pointer whose signature has multi-word types""" + + import cppjit + + cppjit.cppdef(r"""\ + namespace ns56 { + typedef unsigned long (*func_t)(const char *, unsigned int); + unsigned long add_len(const char *s, unsigned int n) { return std::string(s).size() + n; } + func_t get() { return add_len; } + }""") + + # the std::function wrapping the returned pointer is compiled from + # its signature, which must keep "const char" and "unsigned int" + assert cppjit.gbl.ns56.get()("abc", 4) == 7 + class TestANONENUM: def test01_anonymous_enum_repeated_access(self): diff --git a/test/test_regression.py b/test/test_regression.py index 2a2bda4a..40327ac0 100644 --- a/test/test_regression.py +++ b/test/test_regression.py @@ -1727,3 +1727,48 @@ def test56_enum_arg_typedefed_underlying_type(self): assert ns.pick(ns.S.A) == 1 assert ns.pick(ns.S.B) == 5 + + def test57_function_pointer_types_from_ast(self): + """Function pointer return and argument types are taken from the AST""" + + import cppjit + + cppjit.cppdef(r"""\ + namespace FnPtrTypes { + unsigned int one() { return 1u; } + typedef unsigned int (*uf_t)(); + uf_t get_one() { return one; } + unsigned int call(uf_t f) { return f() + 1; } + + double scale(double x, const char* s) { return x * (s[0] - '0'); } + typedef double (*sf_t)(double, const char*); + sf_t get_scale() { return scale; } + double apply(sf_t f, double x, const char* s) { return f(x, s); } + + int noop(int i) noexcept { return i; } + typedef int (*nf_t)(int) noexcept; + nf_t get_noop() { return noop; } + + struct M { int f(double) { return 42; } }; + typedef int (M::*mf_t)(double); + mf_t get_mf() { return &M::f; } + }""") + + ns = cppjit.gbl.FnPtrTypes + + # multi-word return type (was mangled to "unsignedint" by string parsing) + assert ns.get_one()() == 1 + assert ns.call(ns.one) == 2 + assert ns.call(lambda: 5) == 6 + + # multi-word and pointer argument types + assert ns.get_scale()(2.0, "3") == 6.0 + assert ns.apply(ns.scale, 2.0, "4") == 8.0 + assert ns.apply(lambda x, s: x + 1, 2.0, "4") == 3.0 + + # noexcept function pointers + assert ns.get_noop()(7) == 7 + + # member function pointers are not function pointers; they are not + # supported, but must not crash either + ns.get_mf()