From c9e0184f058615b46331cb46ecb4dbdf2a6438e5 Mon Sep 17 00:00:00 2001 From: Pavel Guzenfeld Date: Sun, 7 Jun 2026 00:11:20 +0300 Subject: [PATCH] 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() = 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 --- .../include/rclcpp/any_service_callback.hpp | 84 ++++++------------- rclcpp/include/rclcpp/generic_service.hpp | 51 ++--------- .../test/rclcpp/test_any_service_callback.cpp | 43 ++++++++++ 3 files changed, 72 insertions(+), 106 deletions(-) diff --git a/rclcpp/include/rclcpp/any_service_callback.hpp b/rclcpp/include/rclcpp/any_service_callback.hpp index 5d6f3ee7b7..93903e473e 100644 --- a/rclcpp/include/rclcpp/any_service_callback.hpp +++ b/rclcpp/include/rclcpp/any_service_callback.hpp @@ -33,21 +33,27 @@ namespace rclcpp namespace detail { -template -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 -#ifdef __QNXNTO__ -struct can_be_nullptr() == nullptr)>>: std::true_type {}; -#else -struct can_be_nullptr() == nullptr), decltype(std::declval() = 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 +bool +callback_is_null(const VariantT & callback) +{ + return std::visit( + [](const auto & cb) { + if constexpr (std::is_same_v, 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::value, int> = 0> + template void set(CallbackT && callback) { @@ -102,51 +106,11 @@ class AnyServiceCallback // of all the above workaround ... callback_ = std::forward(callback); } - } - template< - typename CallbackT, - typename std::enable_if_t::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(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrWithRequestHeaderCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallbackWithServiceHandle - >::value) - { - callback_.template emplace(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(callback); - } } // template> diff --git a/rclcpp/include/rclcpp/generic_service.hpp b/rclcpp/include/rclcpp/generic_service.hpp index 8a4468aa1f..1cf32a33bf 100644 --- a/rclcpp/include/rclcpp/generic_service.hpp +++ b/rclcpp/include/rclcpp/generic_service.hpp @@ -23,6 +23,7 @@ #include #include +#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::value, int> = 0> + template void set(CallbackT && callback) { @@ -85,50 +84,10 @@ class GenericServiceCallback // of all the above workaround ... callback_ = std::forward(callback); } - } - template< - typename CallbackT, - typename std::enable_if_t::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(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrWithRequestHeaderCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallbackWithServiceHandle - >::value) - { - callback_.template emplace(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(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"); } } diff --git a/rclcpp/test/rclcpp/test_any_service_callback.cpp b/rclcpp/test/rclcpp/test_any_service_callback.cpp index ef46155d99..767a919e40 100644 --- a/rclcpp/test/rclcpp/test_any_service_callback.cpp +++ b/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, + std::shared_ptr) +{} +} // 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, + std::shared_ptr) = &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, + std::shared_ptr)> 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, + std::shared_ptr) = nullptr; + EXPECT_THROW(any_service_callback_.set(callback), std::invalid_argument); +} + +TEST_F(TestAnyServiceCallback, set_null_std_function_throws) { + std::function, + std::shared_ptr)> callback = nullptr; + EXPECT_THROW(any_service_callback_.set(callback), std::invalid_argument); +}