morganstanley / binlog

A high performance C++ log library, producing structured binary logs
http://opensource.morganstanley.com/binlog/
Apache License 2.0
608 stars 72 forks source link

constexpr check if type is loggable #145

Closed andrewkcorcoran closed 2 years ago

andrewkcorcoran commented 2 years ago

Is it possible to check (in a constexpr context) if a given type is serializable by binlog? Motivating example

template<typename T>
void Log(T && t)
{
   if constexpr(binlog::is_serializable<T>)
   {
      BINLOG_INFO("{}", t);
   }
   else
   {
      BINLOG_INFO("Non serializable type t: {}", boost::typeindex::type_id<T>().pretty_name());
   }
}
erenon commented 2 years ago

It is possible using mserialize::detail::is_serializable, e.g:

https://github.com/morganstanley/binlog/blob/main/test/integration/LoggingBoostTypes.cpp#L63

andrewkcorcoran commented 2 years ago
include/mserialize/detail/Tag.hpp:31:5: error: static_assert failed due to requirement 'always_false<SNIP>::value' "T has no associated tag"

mserialize::detail::is_serializable<TEvent>::value is returning true but then the above static assert is failing? Does is_serializable not check for tags, is it required to also check mserialize::detail::has_tag<T>::value at the same time?

Edit: Event when checking both if constexpr(mserialize::detail::is_serializable<TEvent>::value && mserialize::detail::has_tag<TEvent>::value) include/mserialize/detail/Tag.hpp:31:5: error: static_assert failed still gets triggered. It looks like there might be a difference between what the static_assert is checking and what the constexpr methods are checking.

erenon commented 2 years ago

(Sorry, I was giving the literal answer about checking if the type is serializable, though I should have suspected you are after checking if it is loggable. As you figured out, both is_serializable and has_tag are required.)

Can you give an example of a type T that satisfies has_tag, but mserialize::tag still asserts?

andrewkcorcoran commented 2 years ago

Looks to be when an adapted struct has a composed type which has not been adapted.

Minimal example below.

#include "binlog/binlog.hpp"

struct Nested{};

template<typename TComposed>
struct Composed
{
  TComposed composed;
};
BINLOG_ADAPT_TEMPLATE((typename TComposed), (Composed<TComposed>), composed)

template<typename T>
void Log(const T & t)
{
  if constexpr(mserialize::detail::is_serializable<T>::value && mserialize::detail::has_tag<T>::value)
  {
     BINLOG_INFO("{}", t);
  }
}

int main()
{
  Log(Composed<Nested>{});
}
erenon commented 2 years ago

With BINLOG_ADAPT_STRUCT instead of BINLOG_ADAPT_TEMPLATE the problem becomes more obvious: the fields must be also adapted - this is a (documented) requirement of the macro, but because of the late evaluation of the template, it only blows up on usage (compared to BINLOG_ADAPT_STRUCT, that fails immediately).

A fix I can think of: add static_asserts to BINLOG_ADAPT_TEMPLATE, and make this requirement explicit. Do you have a different use-case that wouldn't be covered by this?

andrewkcorcoran commented 2 years ago

How would a static assert in BINLOG_ADAPT_TEMPLATE work in the above example? BINLOG_ADAPT_TEMPLATE doesn't know what the type of the template is going to be, so therefore it can't assert on the template type having being adapted.

The general requirement here is a way to write a templated log method which will log an event as normal if binlog supports that event or otherwise fall back to just logging the type name or some other generic approach. I think the only way for this to work is if is_serializable and has_tag traverse the full recursive tree of types, the same way the log macros do.

erenon commented 2 years ago

How would a static assert in BINLOG_ADAPT_TEMPLATE work in the above example? BINLOG_ADAPT_TEMPLATE doesn't know what the type of the template is going to be, so therefore it can't assert on the template type having being adapted.

The static_assert can depend on the type of the template parameter, and fire only when the template is instantiated. Do you use BINLOG_ADAPT_TEMPLATE in a way that the fields are may or may not be adapted? If yes, perhaps I can change the generated tag specialization to derive from a constructible type, if every member has a tag, otherwise derive from a non-constructible type - making has_tag work again.

andrewkcorcoran commented 2 years ago

Yes there's no guarantee that the composed type is adaptable. In our use case this, we receive events, wrap them in the template class and then a later stage tries to log them. We don't directly control the events we receive so some of them may have been adapted, some not.

erenon commented 2 years ago

Please see if this works for you: https://github.com/morganstanley/binlog/pull/146

andrewkcorcoran commented 2 years ago

Still seems to be failing on the log line if you add the below to the test case in PR.

int main()
{
    auto e = Nest<NotAdapted>{};
    if constexpr(mserialize::detail::has_tag<Nest<NotAdapted>>::value && mserialize::detail::is_serializable<Nest<NotAdapted>>::value)
    {
        BINLOG_INFO("{}", e);
    }
    return 0;
}
_deps/binlog-src/include/mserialize/detail/Tag.hpp:31:5: error: static_assert failed due to requirement 'always_false<Nest<NotAdapted>>::value' "T has no associated tag"
    static_assert(always_false<T>::value, "T has no associated tag");
    ^             ~~~~~~~~~~~~~~~~~~~~~~
_deps/binlog-src/include/mserialize/tag.hpp:29:32: note: in instantiation of member function 'mserialize::detail::BuiltinTag<Nest<NotAdapted>>::tag_string' requested here
  return detail::Tag<T>::type::tag_string();
                               ^
_deps/binlog-src/include/binlog/create_source_and_event.hpp:67:44: note: in instantiation of function template specialization 'mserialize::tag<Nest<NotAdapted> &>' requested here
  return mserialize::cx_strcat(mserialize::tag<T>()...);
                                           ^
../../source/bin/test.cpp:28:9: note: in instantiation of function template specialization 'binlog::detail::concatenated_tags<char const (&)[3], Nest<NotAdapted> &>' requested here
        BINLOG_INFO("{}", e);
        ^
_deps/binlog-src/include/binlog/basic_log_macros.hpp:37:30: note: expanded from macro 'BINLOG_INFO'
#define BINLOG_INFO(...)     BINLOG_INFO_WC(    binlog::default_thread_local_writer(), main, __VA_ARGS__)
                             ^
_deps/binlog-src/include/binlog/advanced_log_macros.hpp:47:3: note: expanded from macro 'BINLOG_INFO_WC'
  BINLOG_CREATE_SOURCE_AND_EVENT_IF(                                            \
  ^
_deps/binlog-src/include/binlog/create_source_and_event_if.hpp:23:7: note: expanded from macro 'BINLOG_CREATE_SOURCE_AND_EVENT_IF'
      BINLOG_CREATE_SOURCE_AND_EVENT(writer, severity, category, clock, __VA_ARGS__); \
      ^
_deps/binlog-src/include/binlog/create_source_and_event.hpp:51:25: note: expanded from macro 'BINLOG_CREATE_SOURCE_AND_EVENT'
        binlog::detail::concatenated_tags(__VA_ARGS__).data()
erenon commented 2 years ago

According to cppreference, the condition of the if constexpr must depend on a template argument, otherwise the discarded statement is fully checked.

Outside a template, a discarded statement is fully checked. if constexpr is not a substitute for the #if preprocessing directive:

https://en.cppreference.com/w/cpp/language/if

I tested with the original example Log, and it was working.

andrewkcorcoran commented 2 years ago

Ok, slight red herring there - actual failure can be replicated by static_assert(!mserialize::detail::has_tag<std::variant<Nest<NotAdapted>>>::value, "");

erenon commented 2 years ago

Ok, that is a different incarnation of the same omission of the CustomTag defined in adapt_stdvariant.hpp.

erenon commented 2 years ago

Re variant: New testcase and fix added to #146