115

I ran into this while debugging this question.

I trimmed it down all the way to just using Boost Operators:

  1. Compiler Explorer C++17 C++20

    #include <boost/operators.hpp>
    
    struct F : boost::totally_ordered1<F, boost::totally_ordered2<F, int>> {
        /*implicit*/ F(int t_) : t(t_) {}
        bool operator==(F const& o) const { return t == o.t; }
        bool operator< (F const& o) const { return t <  o.t; }
      private: int t;
    };
    
    int main() {
        #pragma GCC diagnostic ignored "-Wunused"
        F { 42 } == F{ 42 }; // OKAY
        42 == F{42};         // C++17 OK, C++20 infinite recursion
        F { 42 } == 42;      // C++17 OK, C++20 infinite recursion
    }
    

    This program compiles and runs fine with C++17 (ubsan/asan enabled) in both GCC and Clang.

  2. When you change the implicit constructor to explicit, the problematic lines obviously no longer compile on C++17

Surprisingly both versions compile on C++20 (v1 and v2), but they lead to infinite recursion (crash or tight loop, depending on optimization level) on the two lines that wouldn't compile on C++17.

Obviously this kind of silent bug creeping in by upgrading to C++20 is worrisome.

Questions:

  • Is this c++20 behaviour conformant (I expect so)
  • What exactly is interfering? I suspect it might be due to c++20's new "spaceship operator" support, but don't understand how it changes the behaviour of this code.
Marco Bonelli
  • 63,369
  • 21
  • 118
  • 128
sehe
  • 374,641
  • 47
  • 450
  • 633
  • 10
    I believe this is [boost::equality_comparable2 operator== can compile into infinite loop with clang10 and -std=c++2a](https://github.com/boostorg/utility/issues/65) – Shafik Yaghmour Jan 10 '21 at 00:23
  • 4
    @ShafikYaghmour You guys are seriously fast. The world does not deserve this level of support. Thank you – sehe Jan 10 '21 at 00:29
  • 1
    I don't think this is a dupe, but it's definitely related https://stackoverflow.com/questions/64130311 – cigien Jan 10 '21 at 00:33
  • 1
    @cigien Appreciated. The explanations in the answers there are excellent and help forming a more complete understanding. – sehe Jan 10 '21 at 00:36

1 Answers1

89

Indeed, C++20 unfortunately makes this code infinitely recursive.

Here's a reduced example:

struct F {
    /*implicit*/ F(int t_) : t(t_) {}

    // member: #1
    bool operator==(F const& o) const { return t == o.t; }

    // non-member: #2
    friend bool operator==(const int& y, const F& x) { return x == y; }

private:
    int t;
};

Let's just look at 42 == F{42}.

In C++17, we only had one candidate: the non-member candidate (#2), so we select that. Its body, x == y, itself only has one candidate: the member candidate (#1) which involves implicitly converting y into an F. And then that member candidate compares the two integer members and this is totally fine.

In C++20, the initial expression 42 == F{42} now has two candidates: both the non-member candidate (#2) as before and now also the reversed member candidate (#1 reversed). #2 is the better match - we exactly match both arguments instead of invoking a conversion, so it's selected.

Now, however, x == y now has two candidates: the member candidate again (#1), but also the reversed non-member candidate (#2 reversed). #2 is the better match again for the same reason that it was a better match before: no conversions necessary. So we evaluate y == x instead. Infinite recursion.

Non-reversed candidates are preferred to reversed candidates, but only as a tiebreaker. Better conversion sequence is always first.


Okay great, how can we fix it? The simplest option is removing the non-member candidate entirely:

struct F {
    /*implicit*/ F(int t_) : t(t_) {}

    bool operator==(F const& o) const { return t == o.t; }

private:
    int t;
};

42 == F{42} here evaluates as F{42}.operator==(42), which works fine.

If we want to keep the non-member candidate, we can add its reversed candidate explicitly:

struct F {
    /*implicit*/ F(int t_) : t(t_) {}
    bool operator==(F const& o) const { return t == o.t; }
    bool operator==(int i) const { return t == i; }
    friend bool operator==(const int& y, const F& x) { return x == y; }

private:
    int t;
};

This makes 42 == F{42} still choose the non-member candidate, but now x == y in the body there will prefer the member candidate, which then does the normal equality.

This last version can also remove the non-member candidate. The following also works without recursion for all test cases (and is how I would write comparisons in C++20 going forward):

struct F {
    /*implicit*/ F(int t_) : t(t_) {}
    bool operator==(F const& o) const { return t == o.t; }
    bool operator==(int i) const { return t == i; }

private:
    int t;
};
Barry
  • 286,269
  • 29
  • 621
  • 977
  • 14
    Excellent walk through. I'm a bit torn on how to judge the situation. As a professional, I'm really unhappy to see C++ introducing compatibility death traps. I'm happy to see [this sentiment](https://github.com/boostorg/utility/issues/65#issuecomment-636073901) but I fear it looks like nothing much is going to come of it. So libraries just silently break - **at runtime** - which is not a good storyline. I know I've written code that will suffer these issues, and I shudder to think what happens if the current devs upgrade. They'll probably be cursing my code in righteous indignation ¯\_(ツ)_/¯ – sehe Jan 10 '21 at 22:40
  • 7
    @sehe I'm really unhappy about it too, and it's my fault even. And this specific case is the worst case - the only language change that preserves the C++17 behavior is not having any part of the feature (or having the `==` functionality opt-in, which nobody has come up with a passable way of doing so). Not only is it the only one we can't really fix, but all the other breakages are code that ceases to compile while this one insidiously continues to compile. Just all sorts of bad. – Barry Jan 10 '21 at 22:50
  • 4
    @sehe And this case even, you could just rewrite the body of `F` to have `auto operator<=>(F const&) const = default;` and that one line gives you all the comparisons. Which is good for C++20 going forward, but not so good for the transitional step obviously. – Barry Jan 10 '21 at 22:51
  • I guess I could have a static inspection that just spots `operator==` implementations and have them routinely conditionally replaced with spaceships in c++20 mode... I do feel the future is a lot better. However, it's a landmine waiting to trip people up. – sehe Jan 11 '21 at 01:46
  • 10
    @Barry I feel like this shouldn't be hard for compilers to statically diagnose (after all, it all happens at compile time). I.e. something like "c++20 compat warning: different overload will be selected depending on the std mode, proposed fix: ...". – Dan M. Jan 11 '21 at 11:22
  • 5
    @Dan I'm not a compiler guy, but if you want to take a crack at adding such a warning to clang, I'm sure many people would be very appreciative. – Barry Jan 11 '21 at 14:40