[Bug libstdc++/96269] New: optional comparison with nullopt fails

[Bug libstdc++/96269] New: optional comparison with nullopt fails

[sshannin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

            Bug ID: 96269
           Summary: optional comparison with nullopt fails
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sshannin at gmail dot com
  Target Milestone: ---

Consider the following:

seth@dev6:~/tmp$ cat opt.cpp
#include <optional>

struct X {
  template <typename T>
  bool operator==(const T&) {
    return false;
  }
};

bool foo() {
  std::optional<X> x;
#ifndef FLIP
  return x == std::nullopt;
#else
  return std::nullopt == x;
#endif
}

With gcc 9.1.0, this compiles regardless of whether or not FLIP is defined.

With gcc 10.1.0 however, the FLIP variant does not compile.

seth@dev6:~/tmp$ /toolchain15/bin/g++ -c -o opt.o opt.cpp --std=c++20
seth@dev6:~/tmp$ /toolchain15/bin/g++ -c -o opt.o opt.cpp --std=c++20 -DFLIP
In file included from opt.cpp:1:
/toolchain15/include/c++/10.1.0/optional: In instantiation of ‘constexpr
std::__optional_relop_t<decltype ((declval<_Up>() == declval<_Tp>()))>
std::operator==(const _Up&, const std::optional<_Tp>&) [with _Tp = X; _Up =
std::nullopt_t; std::__optional_relop_t<decltype ((declval<_Up>() ==
declval<_Tp>()))> = bool; decltype ((declval<_Up>() == declval<_Tp>())) =
bool]’
...etc...

It's a tad hard for me to keep track of which overloads are supposed to
exist/how they're supposed to resolve and I think introduction of <=> was
expected to change behavior here a bit, so I'm not actually sure if this is
supposed to compile still.

seth@dev6:~/tmp$ /toolchain15/bin/g++ -v
Using built-in specs.
COLLECT_GCC=/toolchain15/bin/g++
COLLECT_LTO_WRAPPER=/toolchain15/bin/../libexec/gcc/x86_64-pc-linux-gnu/10.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc_10_1_0/configure --prefix=/toolchain15
--enable-languages=c,c++,fortran --enable-lto --disable-plugin
--program-suffix=-10.1.0 --disable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.1.0 (GCC)

[Bug libstdc++/96269] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Your operator== should be const-qualified.

[Bug libstdc++/96269] optional comparison with nullopt fails

[sshannin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #2 from sshannin at gmail dot com ---
(In reply to Jonathan Wakely from comment #1)
> Your operator== should be const-qualified.

I don't disagree. I can also fully remove the operator== and it compiles as
well (why should the presence of the non-const operator== cause the comparison
with nullopt in one direction to instantiate it).

But yeah, I lost something in my reduction in there. I think the main point is
that one direction of the comparison instantiates the templated operator== and
the other doesn't.

Consider this version of X instead, which avoids the const issues:
struct X {
  int y;

  template <typename T>
  bool operator==(const T&o) const {
    return y == o.summary();
  }
};

We again end up instantiating X's operator== (even though we wouldn't call it)
in the FLIP case (and thus fail to compile), but in the non-flip case
everything seems fine.

[Bug libstdc++/96269] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I don't think this is a bug in std::optional, I think it's how C++20 works.

[Bug libstdc++/96269] optional comparison with nullopt fails

[ville.voutilainen at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

Ville Voutilainen <ville.voutilainen at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ville.voutilainen at gmail dot com

--- Comment #4 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
Right; even gcc trunk is happy with that code in C++17 mode with both values of
FLIP, it's the C++20 spaceship rules that cause trouble here.

[Bug libstdc++/96269] optional comparison with nullopt fails

[ville.voutilainen at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #5 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
Oh, and if you define a spaceship operator for your type, then things work
again, with or without FLIP.

[Bug libstdc++/96269] optional comparison with nullopt fails

[sshannin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #6 from sshannin at gmail dot com ---
Thanks to you both for your analysis. As I said, I wasn't sure if it was an
issue, so I'm certainly willing to accept that it's not.

The one point I wanted to emphasize though just to make sure we're talking
about the same thing is that it seems to odd to me that we're instantiating any
form of comparison function for type T. We're comparing optional<T> to
nullopt_t, so it would seem that it shouldn't matter whether T itself is even
comparable.

More specifically, header <optional> defines operator<=>(const optional<T> &x,
nullopt_t), with the very simple body of bool(x) <=> false, as expected (circa
line 1050, gated by #ifdef __cpp_lib_three_way_comparsion). This is why the
non-flip case works, it hits the spaceship overload for (optional, null_opt_t).
 But there is no such operator in the other direction: (null_opt_t, optional).
So we end up hitting optional's plain templated operator==() at ~line 1120,
which of course ultimately instantiates the T's (broken/weird) operator.

I guess to rephrase, should there also be a specialized spaceship overload for
the (nullopt_t, optional) direction to complement the (optional, nullopt) one?

[Bug libstdc++/96269] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to sshannin from comment #6)
> I guess to rephrase, should there also be a specialized spaceship overload
> for the (nullopt_t, optional) direction to complement the (optional,
> nullopt) one?

No, the compiler synthesizes it from operator==(optional, nullopt_t) by
reversing the arguments. That's how comparisons work in C++20.

[Bug libstdc++/96269] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-11-05
           Keywords|                            |rejects-valid
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Ah, but there is a library bug here after all. The declval expressions used to
constrain the comparison operators are using the wrong type.

They always use declval<_Tp>() == declval<_Up>() but it should be const _Tp&
and const _Up& instead. That would mean those aren't candidates for your
non-const operator== and so the synthesized operator==(nulloptr_t, optional<T>)
would get used.

[Bug libstdc++/96269] optional comparison with nullopt fails

[ville.voutilainen at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #9 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
Ha, well spotted. In general, in a spaceship world, you do want to provide
comparisons symmetrically and const-correctly, and that also works in the
pre-spaceship world, thus:

#include <optional>

struct X {
  template <typename T>
  bool operator==(const T&) const {
    return false;
  }
  template <typename T> friend bool operator==(const T&, const X&) {return
false;}
};

bool foo() {
  std::optional<X> x;
#ifndef FLIP
  return x == std::nullopt;
#else
  return std::nullopt == x;
#endif
}

[Bug libstdc++/96269] [10/11 Regression] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.3
      Known to fail|                            |10.2.1, 11.0
      Known to work|                            |9.3.0
            Summary|optional comparison with    |[10/11 Regression] optional
                   |nullopt fails               |comparison with nullopt
                   |                            |fails
           Priority|P3                          |P2

[Bug libstdc++/96269] [10/11 Regression] optional comparison with nullopt fails

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:cdd2d448d8200ed5ebcb232163954367b553291e

commit r11-4753-gcdd2d448d8200ed5ebcb232163954367b553291e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 5 18:36:19 2020 +0000

    libstdc++: Fix constraints on std::optional comparisons [PR 96269]

    The relational operators for std::optional were using the wrong types
    in the declval expressions used to constrain them. Instead of using
    const lvalues they were using non-const rvalues, which meant that a type
    might satisfy the constraints but then give an error when the function
    body was instantiated.

    libstdc++-v3/ChangeLog:

            PR libstdc++/96269
            * include/std/optional (operator==, operator!=, operator<)
            (operator>, operator<=, operator>=): Fix types used in
            SFINAE constraints.
            * testsuite/20_util/optional/relops/96269.cc: New test.

[Bug libstdc++/96269] [10/11 Regression] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 10.3

[Bug libstdc++/96269] [10/11 Regression] optional comparison with nullopt fails

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96269

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed by r10-8983 for gcc-10