Take function pointer types from the AST instead of parsing type strings

src/cpyrt/Converters.cxx

@@ -26,7 +26,6 @@ using namespace cppjit;
 #include <cstddef>
 #include <limits.h>
 #include <locale> // for wstring_convert
-#include <regex>
 #include <sstream>
 #include <stddef.h> // for ptrdiff_t
 #include <string.h>
@@ -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

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

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<TCppType_t>& 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

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<TCppType_t>& 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<TCppType_t> 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);
 }

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):

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()