Can't use overloaded comparison operator with Catch test

Question

I have a simple unit-test using Catch 2.11.1:

#define CATCH_CONFIG_MAIN
#include "catch.hpp"
#include <utility>
#include <any>

namespace A::B
{
    namespace C
    {
        struct S
        {
        };
    }

    using type = std::pair<C::S, std::any>;
}

inline bool operator==(A::B::type const&, A::B::type const&)
{
    return true;
}

TEST_CASE("test", "[test]")
{
    auto t1 = std::make_pair(A::B::C::S(), std::any());
    auto t2 = std::make_pair(A::B::C::S(), std::any());

    REQUIRE(t1 == t2);
}

The above simple programs generates the following errors:

$ g++ -Wall -Wextra -Wpedantic test-single.cpp -std=c++17
In file included from /usr/include/c++/9/bits/stl_algobase.h:64,
                 from /usr/include/c++/9/bits/char_traits.h:39,
                 from /usr/include/c++/9/string:40,
                 from catch.hpp:457,
                 from test-single.cpp:2:
/usr/include/c++/9/bits/stl_pair.h: In instantiation of ‘constexpr bool std::operator==(const std::pair<_T1, _T2>&, const std::pair<_T1, _T2>&) [with _T1 = A::B::C::S; _T2 = std::any]’:
catch.hpp:2289:98:   required from ‘bool Catch::compareEqual(const LhsT&, const RhsT&) [with LhsT = std::pair<A::B::C::S, std::any>; RhsT = std::pair<A::B::C::S, std::any>]’
catch.hpp:2318:34:   required from ‘const Catch::BinaryExpr<LhsT, const RhsT&> Catch::ExprLhs<LhsT>::operator==(const RhsT&) [with RhsT = std::pair<A::B::C::S, std::any>; LhsT = const std::pair<A::B::C::S, std::any>&]’
test-single.cpp:28:5:   required from here
/usr/include/c++/9/bits/stl_pair.h:449:24: error: no match for ‘operator==’ (operand types are ‘const A::B::C::S’ and ‘const A::B::C::S’)
  449 |     { return __x.first == __y.first && __x.second == __y.second; }
      |              ~~~~~~~~~~^~~~~~~~~~~~

_{[And many many more messages after this...]}

The crucial part of the error message is this line:

/usr/include/c++/9/bits/stl_pair.h: In instantiation of ‘constexpr bool std::operator==(const std::pair<_T1, _T2>&, const std::pair<_T1, _T2>&) [with _T1 = A::B::C::S; _T2 = std::any]’:

From the error message it's clear that it's the standard std::operator== function for std::pair that is being invoked, instead of my overloaded operator== function.

If I don't do the comparison inside the Catch REQUIRE macro, then it works:

auto result = t1 == t2;  // Invokes my overloaded comparison operator
REQUIRE(result);

Now is this a problem with Catch, or with my operator function?

NB: I'm building on Debian SID with a recent build of GCC 9.2

$ g++ --version
g++ (Debian 9.2.1-23) 9.2.1 20200110
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I assume you are aware, but the code as shown is going to fall flat whenever something relies on ADL to find your operator. — Max Langhof, Jan 16 '20 at 13:28
@MaxLanghof In fact now you mention it I believe that's exactly what's happening here — Lightness Races in Orbit, Jan 16 '20 at 13:29
@LightnessRacesBY-SA3.0 Then why does it (according to your answer) work with parentheses? Or does it not? — Max Langhof, Jan 16 '20 at 13:30
@MaxLanghof With the parens it's just a bog-standard `==` and so ADL is not being relied on right? Cos that function is in scope. — Lightness Races in Orbit, Jan 16 '20 at 13:30
@LightnessRacesBY-SA3.0 I mean it doesn't ADL but I guess the operator lookup happens in a different context. It will only fall flat (ADL or not) if there's _any_ other `operator==` declared between the `==` in the code and his user-defined one in the global namespace. But I'm really not inclined to dissect the macro. — Max Langhof, Jan 16 '20 at 13:31
@MaxLanghof Yeah by default a whole bunch of stuff happens in that macro and the expression `t1 == t2` is not actually evaluated in the normal way. My tests are full of added `()` :D — Lightness Races in Orbit, Jan 16 '20 at 13:32
@LightnessRacesBY-SA3.0 Actually ADL applies for "bog-standard" `==`. And unqualified lookup in general. https://en.cppreference.com/w/cpp/language/adl. But I assume the difference comes from dependent name lookup in a template (which does ADL only, see the end of the page) vs normal unqualified lookup. — Max Langhof, Jan 16 '20 at 13:48
@MaxLanghof It applies when it's needed; it's not needed or used here as the operator is in the same scope as the testcase. For the bog-standard `==` (i.e. not the version steeped in a few calls into Catch2 machinery). I do agree with your conclusion though; that sounds reasonable — Lightness Races in Orbit, Jan 16 '20 at 14:38

Max Langhof · Accepted Answer · 2020-01-16T14:26:24.250

Note that even with the parentheses suggested by Lightness, the code you show is exceptionally fragile.

I guess you are originally in ADL-only territory due to dependent name lookup inside the macro (see the last notes of https://en.cppreference.com/w/cpp/language/adl), and your code is quite clearly not ADL-viable. Adding parentheses makes the whole thing just an unqualified lookup, not ADL-only (again, a guess). The non-ADL part of unqualified lookup saves you in this case, but it will fall apart from entirely unrelated code changes.

Consider this code instead of the TEST_CASE, which is what using parentheses presumably boils down to:

namespace test
{
    bool foo()
    {
        auto t1 = std::make_pair(A::B::C::S(), std::any());
        auto t2 = std::make_pair(A::B::C::S(), std::any());

        return t1 == t2;
    }
}

This compiles and works as expected: https://godbolt.org/z/HiuWWy

Now add a completely unrelated operator== between your global operator== and the t1 == t2:

namespace test
{
    struct X{};
    bool operator==(X, X);

    bool foo()
    {
        auto t1 = std::make_pair(A::B::C::S(), std::any());
        auto t2 = std::make_pair(A::B::C::S(), std::any());

        return t1 == t2;
    }
}

And you're out for the count: https://godbolt.org/z/BUQC9Y

The operator== in the global namespace isn't found because the (non-ADL part of) unqualified name lookup stops in the first enclosing scope that has any operator==. Since that doesn't find anything useful, it falls back to using the inbuilt std::pair comparison operator (found via ADL), which won't work.

Just put operator overloads in the namespaces of the objects they operate on. And by corollary, don't overload operators for facilities from std (or other namespaces you are not allowed to touch).

Adding from comments:

The standard currently also says that the namespaces of template arguments are considered, so putting the operator== in namespace C would work (because the first template argument of std::pair comes from there): https://godbolt.org/z/eV8Joj

However, 1. that doesn't mesh too well with your type alias and 2. there is some movement to make ADL less wild and I've seen discussion to get rid of the "consider namespaces of template parameters". See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0934r0.pdf:

Why on earth would we look into the namespaces of template arguments? Nothing in there could possibly be part of the interface of the type, unless the template arguments were also base classes or something. -- Herb Sutter

I don't know where this paper stands today but I would avoid relying on this kind of ADL in new code.

That was my first attempt, to put the operator inside the `A::B` namespace. It still doesn't work, but worse is that then the workaround by Lightness Races BY-SA 3.0 (using parentheses) doesn't work anymore. So yes there's definitely a lot of fragility in my design, and I should probably rethink it a little. — Some programmer dude, Jan 16 '20 at 13:48
@Someprogrammerdude The `operator==` operates on `std::pair`, so it would have to be in `namespace std` to be found. The standard currently also says that the namespaces of template arguments are considered, so putting the `operator==` in `namespace C` would work (because the first template argument of `std::pair` comes from there), but that 1. doesn't mesh too well with your `type` alias and 2. there is some movement to make ADL less wild and the "consider namespaces of template parameters" is on the chopping block afaik. See https://godbolt.org/z/AySzrU, can't find a source for the latter... — Max Langhof, Jan 16 '20 at 13:55
Based on your answer I turned `type` into a new structure and moved the comparison operator into the `A::B` namespace. It works well now. — Some programmer dude, Jan 16 '20 at 14:07

score 5 · Answer 2 · answered Jan 16 '20 at 13:26

The magic that expands the operands in order to provide nice diagnostic output can fall over at times.

The workaround is to disable that with some brackets:

REQUIRE((t1 == t2));

This is effectively the same workaround as you have with the variable.

The documentation mentions this problem in the context of more complex expressions. Exactly why the situation is being triggered in your case I am not certain, but notice from the stack trace how your operator== is not actually being invoked, but instead Catch::BinaryExpr::operator== and Catch::compareEqual, which seems not to have access to (or otherwise chooses not to use) your implementation. Either way, the solution is to disable the decomposition machinery as noted above.

Can't use overloaded comparison operator with Catch test

2 Answers2

Linked