Remove fragile can_be_nullptr trait that broke create_service() (#1742)

The two set() overloads in AnyServiceCallback and GenericServiceCallback were
selected by detail::can_be_nullptr, whose non-QNX specialization probed the raw
callable with decltype(std::declval<T&>() = nullptr). That expression is
ill-formed for some callables/compilers, disabling both overloads and breaking
compilation of create_service() (and the generic service) for affected
callbacks.

Replace the trait with a single set() that stores the callback and then checks
the resulting std::function for emptiness via detail::callback_is_null(), which
visits the variant. std::function's operator bool flags only genuine null
targets (e.g. a null function pointer); lambdas and bind expressions are never
empty, so behavior is preserved while the brittle SFINAE is removed. This also
collapses the duplicated overload pair in both headers.

Add regression tests for function-pointer and std::function callbacks (which
previously failed to compile) and null-rejection for both.

Fixes #1742

Generated-by: Claude Opus 4.8 (Anthropic)
Signed-off-by: Pavel Guzenfeld <pavelguzenfeld@gmail.com>

Author: PavelGuzenfeld

rclcpp/include/rclcpp/any_service_callback.hpp

@@ -33,21 +33,27 @@ namespace rclcpp
 
 namespace detail
 {
-template<typename T, typename = void>
-struct can_be_nullptr : std::false_type {};
-
-// Some lambdas define a comparison with nullptr,
-// but we see a warning that they can never be null when using it.
-// We also test if `T &` can be assigned to `nullptr` to avoid the issue.
-template<typename T>
-#ifdef __QNXNTO__
-struct can_be_nullptr<T, std::void_t<
-    decltype(std::declval<T>() == nullptr)>>: std::true_type {};
-#else
-struct can_be_nullptr<T, std::void_t<
-    decltype(std::declval<T>() == nullptr), decltype(std::declval<T &>() = nullptr)>>
-  : std::true_type {};
-#endif
+// Returns true if the callback stored in the given variant is empty/null.
+// The stored alternatives are std::function objects, whose operator bool is
+// false only when empty; lambdas and bind expressions are never empty, so this
+// flags only genuine null targets (e.g. a null function pointer). Checking the
+// stored std::function avoids probing the raw callable type with `== nullptr` /
+// `= nullptr`, which is ill-formed for some callables and broke compilation of
+// create_service() with certain callbacks/compilers (see ros2/rclcpp#1742).
+template<typename VariantT>
+bool
+callback_is_null(const VariantT & callback)
+{
+  return std::visit(
+    [](const auto & cb) {
+      if constexpr (std::is_same_v<std::decay_t<decltype(cb)>, std::monostate>) {
+        return true;
+      } else {
+        return !cb;
+      }
+    },
+    callback);
+}
 }  // namespace detail
 
 // Forward declare
@@ -62,9 +68,7 @@ class AnyServiceCallback
   : callback_(std::monostate{})
   {}
 
-  template<
-    typename CallbackT,
-    typename std::enable_if_t<!detail::can_be_nullptr<CallbackT>::value, int> = 0>
+  template<typename CallbackT>
   void
   set(CallbackT && callback)
   {
@@ -102,51 +106,11 @@ class AnyServiceCallback
       // of all the above workaround ...
       callback_ = std::forward<CallbackT>(callback);
     }
-  }
 
-  template<
-    typename CallbackT,
-    typename std::enable_if_t<detail::can_be_nullptr<CallbackT>::value, int> = 0>
-  void
-  set(CallbackT && callback)
-  {
-    if (!callback) {
+    // Reject a null target (e.g. a null function pointer or empty std::function).
+    if (detail::callback_is_null(callback_)) {
       throw std::invalid_argument("AnyServiceCallback::set(): callback cannot be nullptr");
     }
-    // Workaround Windows issue with std::bind
-    if constexpr (
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrCallback
-      >::value)
-    {
-      callback_.template emplace<SharedPtrCallback>(callback);
-    } else if constexpr (  // NOLINT
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrWithRequestHeaderCallback
-      >::value)
-    {
-      callback_.template emplace<SharedPtrWithRequestHeaderCallback>(callback);
-    } else if constexpr (  // NOLINT
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrDeferResponseCallback
-      >::value)
-    {
-      callback_.template emplace<SharedPtrDeferResponseCallback>(callback);
-    } else if constexpr (  // NOLINT
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrDeferResponseCallbackWithServiceHandle
-      >::value)
-    {
-      callback_.template emplace<SharedPtrDeferResponseCallbackWithServiceHandle>(callback);
-    } else {
-      // the else clause is not needed, but anyways we should only be doing this instead
-      // of all the above workaround ...
-      callback_ = std::forward<CallbackT>(callback);
-    }
   }
 
   // template<typename Allocator = std::allocator<typename ServiceT::Response>>

rclcpp/include/rclcpp/generic_service.hpp

@@ -23,6 +23,7 @@
 #include <utility>
 #include <variant>
 
+#include "rclcpp/any_service_callback.hpp"
 #include "rclcpp/typesupport_helpers.hpp"
 
 #include "rosidl_runtime_c/service_type_support_struct.h"
@@ -45,9 +46,7 @@ class GenericServiceCallback
   : callback_(std::monostate{})
   {}
 
-  template<
-    typename CallbackT,
-    typename std::enable_if_t<!detail::can_be_nullptr<CallbackT>::value, int> = 0>
+  template<typename CallbackT>
   void
   set(CallbackT && callback)
   {
@@ -85,50 +84,10 @@ class GenericServiceCallback
       // of all the above workaround ...
       callback_ = std::forward<CallbackT>(callback);
     }
-  }
 
-  template<
-    typename CallbackT,
-    typename std::enable_if_t<detail::can_be_nullptr<CallbackT>::value, int> = 0>
-  void
-  set(CallbackT && callback)
-  {
-    if (!callback) {
-      throw std::invalid_argument("AnyServiceCallback::set(): callback cannot be nullptr");
-    }
-    // Workaround Windows issue with std::bind
-    if constexpr (
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrCallback
-      >::value)
-    {
-      callback_.template emplace<SharedPtrCallback>(callback);
-    } else if constexpr (  // NOLINT
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrWithRequestHeaderCallback
-      >::value)
-    {
-      callback_.template emplace<SharedPtrWithRequestHeaderCallback>(callback);
-    } else if constexpr (  // NOLINT
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrDeferResponseCallback
-      >::value)
-    {
-      callback_.template emplace<SharedPtrDeferResponseCallback>(callback);
-    } else if constexpr (  // NOLINT
-      rclcpp::function_traits::same_arguments<
-        CallbackT,
-        SharedPtrDeferResponseCallbackWithServiceHandle
-      >::value)
-    {
-      callback_.template emplace<SharedPtrDeferResponseCallbackWithServiceHandle>(callback);
-    } else {
-      // the else clause is not needed, but anyways we should only be doing this instead
-      // of all the above workaround ...
-      callback_ = std::forward<CallbackT>(callback);
+    // Reject a null target (e.g. a null function pointer or empty std::function).
+    if (detail::callback_is_null(callback_)) {
+      throw std::invalid_argument("GenericServiceCallback::set(): callback cannot be nullptr");
     }
   }
 

rclcpp/test/rclcpp/test_any_service_callback.cpp

@@ -109,3 +109,46 @@ TEST_F(TestAnyServiceCallback, set_and_dispatch_defered_with_service_handle) {
     EXPECT_EQ(nullptr, any_service_callback_.dispatch(nullptr, request_header_, request_)));
   EXPECT_EQ(callback_with_header_calls, 1);
 }
+
+namespace
+{
+void free_service_callback(
+  const std::shared_ptr<test_msgs::srv::Empty::Request>,
+  std::shared_ptr<test_msgs::srv::Empty::Response>)
+{}
+}  // namespace
+
+// Regression for ros2/rclcpp#1742: setting the callback with a plain function
+// pointer or std::function must compile and work. The previous can_be_nullptr
+// SFINAE trait was ill-formed for some callables and broke create_service().
+TEST_F(TestAnyServiceCallback, set_and_dispatch_function_pointer) {
+  void (* callback)(
+    const std::shared_ptr<test_msgs::srv::Empty::Request>,
+    std::shared_ptr<test_msgs::srv::Empty::Response>) = &free_service_callback;
+  EXPECT_NO_THROW(any_service_callback_.set(callback));
+  EXPECT_NO_THROW(
+    EXPECT_NE(nullptr, any_service_callback_.dispatch(nullptr, request_header_, request_)));
+}
+
+TEST_F(TestAnyServiceCallback, set_and_dispatch_std_function) {
+  std::function<void(
+      const std::shared_ptr<test_msgs::srv::Empty::Request>,
+      std::shared_ptr<test_msgs::srv::Empty::Response>)> callback = free_service_callback;
+  EXPECT_NO_THROW(any_service_callback_.set(callback));
+  EXPECT_NO_THROW(
+    EXPECT_NE(nullptr, any_service_callback_.dispatch(nullptr, request_header_, request_)));
+}
+
+TEST_F(TestAnyServiceCallback, set_null_function_pointer_throws) {
+  void (* callback)(
+    const std::shared_ptr<test_msgs::srv::Empty::Request>,
+    std::shared_ptr<test_msgs::srv::Empty::Response>) = nullptr;
+  EXPECT_THROW(any_service_callback_.set(callback), std::invalid_argument);
+}
+
+TEST_F(TestAnyServiceCallback, set_null_std_function_throws) {
+  std::function<void(
+      const std::shared_ptr<test_msgs::srv::Empty::Request>,
+      std::shared_ptr<test_msgs::srv::Empty::Response>)> callback = nullptr;
+  EXPECT_THROW(any_service_callback_.set(callback), std::invalid_argument);
+}