archibate / debug-hpp

printing everything including STL containers without pain 🚀
44 stars 3 forks source link

debug_cond_is_range Fails for Container Type #6

Open stellarpower opened 1 month ago

stellarpower commented 1 month ago

Hi,

Nice library; I'm having some real issues though with a container type. I'm using FixedVector from the fixed-containers library. The underlying storage has a fixed size, so tuple_size_v is well-defined and is 0 on this type. Therefore, the print formatter is resolving it as an empty tuple, and printing out {}, instead of a std-compatible container and iterating over the elements.

What has my head completely spinning though, is that the iterator functions are well-defined and working on it, but the trait for if it's a range is false. I have compared it side-by-side with std::vector:

using FixedVector = fixed_containers::FixedVector<int, 16>;

using ComparisonFixed = decltype(begin(std::declval<FixedVector      const &>()) != end(std::declval<FixedVector      const &>()));
using ComparisonStd   = decltype(begin(std::declval<std::vector<int> const &>()) != end(std::declval<std::vector<int> const &>()));

static_assert(std::is_same_v<ComparisonFixed, double>, "Fixed type of inequality comparison");
static_assert(std::is_same_v<ComparisonStd  , double>, "std   type of inequality comparison");
// both are printed as bool.
// debug.hpp:1055:15: error: static assertion failed due to requirement 'std::is_same_v<bool, double>': Fixed type of inequality comparison
//  1055 | static_assert(std::is_same_v<ComparisonFixed, double>, "Fixed type of inequality comparison");
//       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// debug.hpp:1056:15: error: static assertion failed due to requirement 'std::is_same_v<bool, double>': std   type of inequality comparison
//  1056 | static_assert(std::is_same_v<ComparisonStd  , double>, "std   type of inequality comparison");
//       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

using VoidFixed = typename debug_void<ComparisonFixed>::type;
using VoidStd   = typename debug_void<ComparisonStd  >::type;

static_assert(std::is_same_v<VoidFixed, double>, "Fixed type of debug_void of decltype of comparison");
static_assert(std::is_same_v<VoidStd  , double>, "std   type of debug_void of decltype of comparison");
// Both are void
// debug.hpp:1062:15: error: static assertion failed due to requirement 'std::is_same_v<void, double>': Fixed type of debug_void of decltype of comparison
//  1062 | static_assert(std::is_same_v<VoidFixed, double>, "Fixed type of debug_void of decltype of comparison");
//       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// debug.hpp:1063:15: error: static assertion failed due to requirement 'std::is_same_v<void, double>': std   type of debug_void of decltype of comparison
//  1063 | static_assert(std::is_same_v<VoidStd  , double>, "std   type of debug_void of decltype of comparison");

static_assert(is_same_v<decltype(debug_cond_is_range<FixedVector     >::value), double>, "Fixed type of trait requirement");
static_assert(is_same_v<decltype(debug_cond_is_range<std::vector<int>>::value), double>, "std   type of trait requirement");
// debug.hpp:1066:15: error: static assertion failed due to requirement 'is_same_v<const bool, double>': Fixed type of trait requirement
//  1066 | static_assert(is_same_v<decltype(debug_cond_is_range<FixedVector >::value), double>, "Fixed type of trait requirement");
//       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// debug.hpp:1067:15: error: static assertion failed due to requirement 'is_same_v<const bool, double>': std   type of trait requirement
//  1067 | static_assert(is_same_v<decltype(debug_cond_is_range<std::vector<int>>::value), double>, "std   type of trait requirement");
// 

static_assert(debug_cond_is_range<FixedVector     >::value == 999, "Fixed is a range");
static_assert(debug_cond_is_range<std::vector<int>>::value == 999, "std   is a range");
// And now one is true, and the other false. 

I'm really struggling to folow the logic of what's meant to happen here.

debug_cond_is_range expands to

template <class T, class = void> struct debug_cond_is_range : std::false_type {};

template <class T> struct debug_cond_is_range<
    T, 
    typename debug_void<
        decltype(  begin(std::declval<T const &>()) != end(std::declval<T const &>())  )
    >::type
> : std::true_type {};

which surely means that if debug_void<....>::type exists on both of them, the true_type version is well-defined and selected. Both are debug_cond_is_range<T, void>, yet evidently true_type is being selected for the std::vector and false_type for the FixedVector. All whilst both have bool as the type of the iterator comparison, and both have void as the nested type.

If the traits ultimately have to be specialised, I think it'd be much clearer simply to use a boolean non-type argument rather than inheriting from bool_constant. Or some clarification on what should be expected, because defaulting to void and also using it as a nested type adds some confusion. void to me indicates a failure to expand something, and so should be falsey, yet it seems it's being used as the nested type to denote success in resolving the type of the iterator comparison and so is truthy. Or, using constraints would ideally be much more readable, but I understand this would force C++20 support and so may not be acceptable.

But, is this as expected? Admittedly I'm going a bit screen-blind right now, but I'm struggling to see how the compiler is able to select a different specialisation when the comparison is well-defined and thus debug_void<....>::type is defined for both cases.

I also did this directly in the header file at the print specialisation - just in case there's some name lookup issue. Are begin/end custom, or did the std:: get missed off? I don't see a definition in the header file so that seems a potential point of difference, but still not following how they are both deduced as bool. I am using the xmake package, so on #ec10419.

Thanks!

archibate commented 1 month ago

Backgrounds

C++ has a mechanism called argument-depandent lookup (ADL). That is, when invoking unqualified free-functions on objects, the namespace within which the object is defined will be take in to account when looking up for the name of the free-function (not only lookup in the global namespace), e.g.:

namespace ns {
struct Object {};
void function(Object o) {}
}

ns::Object o;
function(o);
// equivalent to:
ns::function(o);

ADL is used to achieve static polymorphism, as you may have multiple classes within their own namespaces and free-functions are placed inside namespaces, invoke only with unqualified name to achieve static dispatch.

For your problem

Thus, when vec is std::vector, begin(vec) is actually automatically deduce as std::begin(vec).

However, if vec is fixed_containers::FixedVector, then begin(vec) will first try to deduced as fixed_containers::begin.

However, it seems that they didn't define such a free-function named begin in their namespace fixed_containers. That's why the expression begin(vec) != end(vec) failed to compile when vec is fixed_containers::FixedVector, making debug_is_cond_range evaluate to false.

So, fixed_containers::FixedVector only supports accessing begin and end iterator with member functions vec.begin() and vec.end(). You may woundering why I didn't use vec.begin() != vec.end() as test expression for debug_is_cond_range, that's because I'd like to support C-style array as printable range too. std::begin and std::end supports getting starting and ending iterators (actually pointers) of const T[] arrays. And this is why C++17 range-based loops (for (auto x: arr)) syntax supports C-style arrays, which definitely have no .begin() or .end() as member functions.

Solution

Sorry! My fault for assuming all container libraries to have free-functions named begin and end in the same namespace in which their container class are defined.

The good news is, std::begin can automatically detect if the argument type are classes and have member .begin() and .end(), if they are not C-arrays.

So the solution might be changing begin(...) into std::begin, and everything will work well (including C-arrays and td::vectors), while making debug_cond_is_range compatible to libraries like fixed_containers who didn't feel like defining begin and end as free-functions.

using ComparisonFixed = decltype(std::begin(std::declval<FixedVector      const &>()) != std::end(std::declval<FixedVector      const &>()));
using ComparisonStd   = decltype(std::begin(std::declval<std::vector<int> const &>()) != std::end(std::declval<std::vector<int> const &>()));

As of constraints

Actually, debug-hpp did used C++20 concepts for checking ranges and other requirements.

The story is, debug-hpp actually have been originated from my other library co_async which is about C++20 coroutines and forcing C++20 anyway.

https://github.com/archibate/co_async/blob/bedfde33de2fbc8265167afc6dd675fe515e25d4/co_async/utils/debug.hpp#L672-674

However, after extracted debug-hpp as an individual project, many debug-hpp users argues C++17 or even C++11 support, which means no concepts or require clause anymore.

I used to try maintain two version of debug-hpp: when C++20 detected, switch to the "concept version" and vice versa. But switching back and forth takes too much maintanance efforts and soon I end up only keeping the SFINAE version till now...

stellarpower commented 1 month ago

Okay okay, I've got you. It was only halfway through writing the issue and after all that debugging I clocked the unqualified begin and end (I'm usually writing for myself/a small team so I always just using namespace std and only bother putting things in a namespace if I'm running into collisions). I'm still not 100% why the constraint on its own is okay (i.e. begin( declval<T>{} )... is fine on its own) but doesn't really matter, I'm just gonna assume it's a scoping thing, so because we're in the function the namespace has been pulled in but when evaluating the arguments for the struct specialisation is different scopt. Whatev, problem solved ;)

I entirely agree on the pointers, and agree it's inconvenient but a bit useless maintaining enable_if alongside concepts unless in tandem. Unfortunate there's not a nice macro or something that can generate the equivalent. Not googled it before but maybe someone has pulled out some magic. So I think probably the best way to handle it would be to expand that "concept" to include more possibilities - .begin/.end as member functions, qualified std::begin/std::end. That way all options are handled, you can pull in anything that might be in the namespace of the type. Then I guess a helper struct that provides the preferred (in the sense of the one that evaluated to true first) way to start and finish on the container, somewhat in the same way as the std:: free functions themselves do. We should probably also be sure that we are handling cbegin and cend too. It shouldn't matter in the sense that it's the type rather than if it's const or not, but maybe something out there only supports read access and only exposes those. Might be worth testing it against some boost containers or similar non-std containers that are commonly used, as there is an annoying variety of different stuff out there, in case you haven't already.

I guess I'm also curious as to why it was deduced as being tuple-like. That may well just be one quirk of this particular container and I'm happy to write it off if so, didn't bother investigating it at the time, but I wonder if other containers out there might suffer from the same problem. It's not too uncommon to wrap something around a tuple for runtime access, I was using debug-hpp in a project autogenerating some boilerplate, so I have a lot of stuff with heterogeneous types wrapped up in more normal containers for runtime access (hence FixedVector, it's constexpr-suitable so I can iterate over a typelist, spit out my boilerplate, upcast, and then the other side just accesses the runtime portion that's in the parent class).

If you're interested in coroutines BTW, it's a couple of years since I last used them but following from this blog post they're very useful when combined with ranges for lazily evaluating an expression tree - in this case, simulating a quantum computer where the matrix size gets large very quickly. Whilst often used for generators or asynchronous/network stuff, in this case we just stash the operation and return the coroutine, and later, we can manipulate the tree e.g. to balance it, walk the tree and apply them I had to bind to Python so limited in what could be done. IT shouldbe somewhere in here. That was an assessed academic group project - so I didn't get to finish it nearly as much as I'd have liked within time constraints to and I think the code finally pushed up is a bit messy and may be pretty incomplete. I probably have some half-baked stuff lingering on my hard drive. Ultimately I'd have liked to have gone through the tree and looked for zeros - there are often lots of them, and the core matrices are generally 22 or similar, so we can in theory prune large tensor products or matrix multiplications knowing that the upstream result isn't going to be used when one entry in the 22 is zero, so we can forget about the whole branch. Unfortunately, ranges as they stand aren't so easy on 2D or higher dimensions, and it can also be limited as they're not really usefully returnable, the only base type currently is pretty inefficent so you can lose the benefit if you're not actually evaluating the end result all within the same function or able to use templates all the way through (cause of Python, had to use runtime polymorpism). But maybe one day.

Thanks for the detailed reply! Appreciate it.