AnySubscriptionCallback doesn't accept `std::bind` callbacks with bound arguments about rclcpp HOT 9 OPEN

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rclcpp-template-metaprogramming-bug-help-wanted/36319/1

from rclcpp.

@DensoADAS or @jmachowinski can you take a look at this? I read the error and nothing obvious jumped out at me. Maybe you have more context after landing #1928

from rclcpp.

I'll have a look.

from rclcpp.

@mjcarroll I looked into this, as far as I can say, the argument deduction for std::bind in
https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/function_traits.hpp
is broken. For some reason,

class SomeClass
{

public:

  struct SomeHandle
  {
    int i;
  };

  void rosMessageHandler(const int& /*channelId*/,
                          SomeHandle /*clientHandle*/,
                          std::shared_ptr<rclcpp::SerializedMessage> /*msg*/) {
  }
};

rclcpp::function_traits::function_traits<std::bind(&SomeClass::rosMessageHandler, &s, 1, SomeClass::SomeHandle(), std::placeholders::_1)>::arguments

resolves to

std::tuple<int const&, SomeClass::SomeHandle, std::shared_ptr<rclcpp::SerializedMessage> >

instead of the expected

std::shared_ptr<rclcpp::SerializedMessage>

But this is serious meta programming foo, which isn't my best discipline. Any Ideas ?

from rclcpp.

@mjcarroll https://github.com/cellumation/rclcpp/tree/function_traits_bug
This branch contains a test, that shows the bug, and currently fails.

from rclcpp.

@mjcarroll Something I forgot to mention: As to why this bug now surfaced:
Before the interface to create_generic_subscription did not forward the given callback
object directly, but converted it to std::function<void(std::shared_ptrrclcpp::SerializedMessage)>.
Therefore the function_trait was never call with the original bind result.

from rclcpp.

I guess in this (and related) blocks

// std::bind for object methods
template<typename ClassT, typename ReturnTypeT, typename ... Args, typename ... FArgs>
#if defined DOXYGEN_ONLY
struct function_traits<std::bind<ReturnTypeT (ClassT::*)(Args ...), FArgs ...>>
#elif defined _LIBCPP_VERSION  // libc++ (Clang)
struct function_traits<std::__bind<ReturnTypeT (ClassT::*)(Args ...), FArgs ...>>
#elif defined _GLIBCXX_RELEASE  // glibc++ (GNU C++ >= 7.1)
struct function_traits<std::_Bind<ReturnTypeT(ClassT::* (FArgs ...))(Args ...)>>
#elif defined __GLIBCXX__  // glibc++ (GNU C++)
struct function_traits<std::_Bind<std::_Mem_fn<ReturnTypeT (ClassT::*)(Args ...)>(FArgs ...)>>
#elif defined _MSC_VER  // MS Visual Studio
struct function_traits<
  std::_Binder<std::_Unforced, ReturnTypeT (ClassT::*)(Args ...), FArgs ...>>
#else
#error "Unsupported C++ compiler / standard library"
#endif
  : function_traits<ReturnTypeT(Args ...)>
{};

you need to write some meta-function which iterates through FArgs... and for every placeholder it needs to place the corresponding element of Args... at position std::is_placeholder<FArg>::value of the resulting arguments tuple. This may work for "sane" use cases, but e.g., std::bind(&CLS::Func, object, _1, _1) could break this (the bind construct is valid as long as the parameter passed to _1 can be converted to both arguments of CLS::Func).

from rclcpp.

Very crude proof-of-concept: https://godbolt.org/z/7oWfYTEqh
Feel free to improve that. Especially for MSVC I have no idea, why the std::is_placeholder does not work properly -- it seems std::bind replaces the placeholders by some other type. I'm also not sure if the order of Args... vs FArgs... is correct for all compilers.

from rclcpp.

The MSVC issue can easily be solved by:

std::is_placeholder<std::remove_reference_t<FArg> >::value

Adding this does not hurt other compilers: https://godbolt.org/z/sP63PMdMM

I won't spend time working on a cleaned-up solution (at least not in the foreseeable future).

from rclcpp.