Fix create_service() compile failure from can_be_nullptr trait (#1742)

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);
+}