Bug 96269 - [10/11 Regression] optional comparison with nullopt fails
Summary: [10/11 Regression] optional comparison with nullopt fails
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 10.1.0
: P2 normal
Target Milestone: 10.3
Assignee: Jonathan Wakely
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2020-07-21 14:44 UTC by sshannin
Modified: 2020-11-05 19:38 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 9.3.0
Known to fail: 10.2.1, 11.0
Last reconfirmed: 2020-11-05 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description sshannin 2020-07-21 14:44:47 UTC
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)
Comment 1 Jonathan Wakely 2020-07-21 15:10:35 UTC
Your operator== should be const-qualified.
Comment 2 sshannin 2020-07-21 16:17:43 UTC
(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.
Comment 3 Jonathan Wakely 2020-11-05 15:40:38 UTC
I don't think this is a bug in std::optional, I think it's how C++20 works.
Comment 4 Ville Voutilainen 2020-11-05 15:52:20 UTC
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.
Comment 5 Ville Voutilainen 2020-11-05 16:07:25 UTC
Oh, and if you define a spaceship operator for your type, then things work again, with or without FLIP.
Comment 6 sshannin 2020-11-05 16:51:17 UTC
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?
Comment 7 Jonathan Wakely 2020-11-05 17:41:51 UTC
(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.
Comment 8 Jonathan Wakely 2020-11-05 17:50:23 UTC
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.
Comment 9 Ville Voutilainen 2020-11-05 17:54:28 UTC
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
}
Comment 10 GCC Commits 2020-11-05 19:09:27 UTC
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.
Comment 11 Jonathan Wakely 2020-11-05 19:31:33 UTC
Fixed for 10.3
Comment 12 Jonathan Wakely 2020-11-05 19:38:15 UTC
Fixed by r10-8983 for gcc-10