[Bug libstdc++/104606] New: Regression in comparison operator resolution with std::optional

[Bug libstdc++/104606] New: Regression in comparison operator resolution with std::optional

[julien.philippon at epitech dot eu]

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

            Bug ID: 104606
           Summary: Regression in comparison operator resolution with
                    std::optional
           Product: gcc
           Version: 11.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: julien.philippon at epitech dot eu
  Target Milestone: ---

Created attachment 52480
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52480&action=edit
Preprocessed file trigering the regression

This code used to compile with G++ 11.1 but does not anymore with G++ 11.2 :

```
#include <optional>
#include <variant>
#include <vector>

struct Value : public std::variant<int, std::vector<Value>> {
    using variant::variant;
};

struct Comparator {
    template <typename T>
    bool operator<=(const T &rhs)
    {
        return true;
    }
};

int main()
{
    auto test = Comparator() <= Value{1};
    auto test2 = Comparator() <= std::make_optional(Value{1});
}
```

In GCC 11.1 the comparison correctly deduced in the two cases that the lower or
equal operator of Comparator should be used, but in GCC 11.2, the second
comparison which involve an optional fails to compile.

It looks like GCC is trying to instantiate the three-way comparison operator of
Value, instead of using the operator of Comparator.

Output from g++ -v :

```
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/11.2.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr --libdir=/usr/lib
--libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info
--with-bugurl=https://bugs.archlinux.org/
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++,d --with-isl
--with-linker-hash-style=gnu --with-system-zlib --enable-__cxa_atexit
--enable-cet=auto --enable-checking=release --enable-clocale=gnu
--enable-default-pie --enable-default-ssp --enable-gnu-indirect-function
--enable-gnu-unique-object --enable-linker-build-id --enable-lto
--enable-multilib --enable-pgo-build=lto --enable-plugin --enable-shared
--enable-threads=posix --disable-libssp --disable-libstdcxx-pch
--disable-werror --with-build-config=bootstrap-lto
--enable-link-serialization=1 gdc_include_dir=/usr/include/dlang/gdc
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.2.0 (GCC)
```

[Bug libstdc++/104606] Regression in comparison operator resolution with std::optional

[julien.philippon at epitech dot eu]

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

--- Comment #1 from julien.philippon at epitech dot eu ---
I forgot to add that the bug triggers only when using C++ 20.

When compiling with `g++ comparison_bug.cxx` the code compiles, but when
compiling with `g++ -std=c++20 comparison_bug.cxx` this bug happens.

[Bug c++/104606] [11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|libstdc++                   |c++
   Target Milestone|---                         |11.3
           Keywords|                            |rejects-valid
            Summary|Regression in comparison    |[11/12 Regression]
                   |operator resolution with    |comparison operator
                   |std::optional               |resolution with
                   |                            |std::optional and
                   |                            |-std=c++20

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I am suspecting a front-end change caused this.
clang accepts the same code with libstdc++ which is another reason why I think
it might be a front-end issue.

[Bug c++/104606] [11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
With preprocessed source from 11.1 even latest 11.2.1 compiles it fine with
-std=c++20 (and also trunk).

[Bug c++/104606] [11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Just wild guess, perhaps the PR98842 changes between 11.1 and 11.2?

[Bug libstdc++/104606] [10/11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |10.3.0
            Summary|[11/12 Regression]          |[10/11/12 Regression]
                   |comparison operator         |comparison operator
                   |resolution with             |resolution with
                   |std::optional and           |std::optional and
                   |-std=c++20                  |-std=c++20
          Component|c++                         |libstdc++
   Target Milestone|11.3                        |10.4

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> Just wild guess, perhaps the PR98842 changes between 11.1 and 11.2?

That would mean it is a bug in GCC 10.4 also :(.

[Bug libstdc++/104606] [10/11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I've confirmed that removing those 2 lines:
   template<typename _Tp, typename _Up>
-    requires (!__is_optional_v<_Up>)
-      && three_way_comparable_with<_Tp, _Up>
     constexpr compare_three_way_result_t<_Tp, _Up>
     operator<=>(const optional<_Tp>& __x, const _Up& __v)
     { return bool(__x) ? *__x <=> __v : strong_ordering::less; }
makes it accepted again.
That doesn't mean it doesn't need to be a FE bug, or it could not be a bug at
all, we need some C++ lawyer for that.

[Bug libstdc++/104606] [10/11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Because what libstdc++ does there seems to match the standard:
https://eel.is/c++draft/optional.comp.with.t#25

[Bug libstdc++/104606] [10/11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug libstdc++/104606] [10/11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
See https://wg21.link/lwg3566 which is what the PR98842 fix does.

[Bug libstdc++/104606] [10/11/12/13 Regression] comparison operator resolution with std::optional and -std=c++20

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug libstdc++/104606] [11/12/13/14 Regression] comparison operator resolution with std::optional and -std=c++20

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.

[Bug libstdc++/104606] [11/12/13/14 Regression] comparison operator resolution with std::optional and -std=c++20

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-03-27
             Status|UNCONFIRMED                 |NEW

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> I am suspecting a front-end change caused this.
> clang accepts the same code with libstdc++ which is another reason why I
> think it might be a front-end issue.

And clang accepts the example when using libc++, which also implements LWG
3566. So I don't think the LWG 3566 change itself is the problem.

[Bug libstdc++/104606] [11/12/13/14 Regression] comparison operator resolution with std::optional and -std=c++20

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
std::optional's spaceship operator currently looks like this:

  template<typename _Tp, typename _Up>
    requires (!__is_optional_v<_Up>)
      && three_way_comparable_with<_Tp, _Up>
    constexpr compare_three_way_result_t<_Tp, _Up>
    operator<=>(const optional<_Tp>& __x, const _Up& __v)

As Jakub noted, if we revert the LWG 3566 change it works:

  template<typename _Tp, three_way_comparable_with<_Tp> _Up>
    constexpr compare_three_way_result_t<_Tp, _Up>
    operator<=>(const optional<_Tp>& __x, const _Up& __v)

We can rewrite that to this, which should be exactly equivalent, and it still
works:

  template<typename _Tp, typename _Up>
    requires three_way_comparable_with<_Up, _Tp>
    constexpr compare_three_way_result_t<_Tp, _Up>
    operator<=>(const optional<_Tp>& __x, const _Up& __v)

and then we can add the !is-optional<U> constraint back in:

  template<typename _Tp, typename _Up>
    requires
    (!__is_optional_v<_Up>) &&
      three_way_comparable_with<_Up, _Tp>
    constexpr compare_three_way_result_t<_Tp, _Up>
    operator<=>(const optional<_Tp>& __x, const _Up& __v)

And it still works! The only difference is:
  three_way_comparable_with<_Up, _Tp>
vs
  three_way_comparable_with<_Tp, _Up>

This should not matter, but it does.

So there definitely seems to be a front end bug here, but at least there's an
easy fix for the library.

[Bug libstdc++/104606] [11/12/13/14 Regression] comparison operator resolution with std::optional and -std=c++20

[redi at gcc dot gnu.org]

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

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #5)
> (In reply to Jakub Jelinek from comment #4)
> > Just wild guess, perhaps the PR98842 changes between 11.1 and 11.2?
> 
> That would mean it is a bug in GCC 10.4 also :(.

It's not, even though 10.4 has the library change. Compiling the test case with
10.4 works. But preprocessing with 10.4 and then compiling with 11.1 fails,
confirming a front end change is to blame.

Bisection shows it started to error with r11-2774

c++: Check satisfaction before non-dep convs. [CWG2369]

It's very hard to use concepts to protect a template from hard errors due to
unwanted instantiation if constraints aren't checked until after doing all
substitution and checking of non-dependent conversions.

It was pretty straightforward to insert the satisfaction check into the
logic, but I needed to make the 3-parameter version of
satisfy_declaration_constraints call push_tinst_level like the 2-parameter
version already does.  For simplicity, I also made it add any needed outer
template arguments from the TEMPLATE_DECL to the args.

The testsuite changes are mostly because this change causes unsatisfaction
to cause deduction to fail rather than reject the candidate later in
overload resolution.

gcc/cp/ChangeLog:

DR 2369
* cp-tree.h (push_tinst_level, push_tinst_level_loc): Declare.
* constraint.cc (satisfy_declaration_constraints):
Use add_outermost_template_args and push_tinst_level.
* pt.c (add_outermost_template_args): Handle getting
a TEMPLATE_DECL as the first argument.
(push_tinst_level, push_tinst_level_loc): No longer static.
(fn_type_unification): Check satisfaction before non-dependent
conversions.

[Bug libstdc++/104606] [11/12/13/14 Regression] comparison operator resolution with std::optional and -std=c++20

[cvs-commit at gcc dot gnu.org]

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

--- Comment #14 from GCC 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:7f65d8267fbfd19cf21a3dc71d27e989e75044a3

commit r14-9771-g7f65d8267fbfd19cf21a3dc71d27e989e75044a3
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Mar 27 21:51:13 2024 +0000

    libstdc++: Reverse arguments in constraint for std::optional's <=>
[PR104606]

    This is a workaround for a possible compiler bug that causes constraint
    recursion in the operator<=>(const optional<T>&, const U&) overload.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104606
            * include/std/optional (operator<=>(const optional<T>&, const U&)):
            Reverse order of three_way_comparable_with template arguments.
            * testsuite/20_util/optional/relops/104606.cc: New test.

[Bug libstdc++/104606] [11/12/13 Regression] comparison operator resolution with std::optional and -std=c++20

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3d16f8f2aec9583422d00c531732ca9d33e6ef26

commit r13-8674-g3d16f8f2aec9583422d00c531732ca9d33e6ef26
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Mar 27 21:51:13 2024 +0000

    libstdc++: Reverse arguments in constraint for std::optional's <=>
[PR104606]

    This is a workaround for a possible compiler bug that causes constraint
    recursion in the operator<=>(const optional<T>&, const U&) overload.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104606
            * include/std/optional (operator<=>(const optional<T>&, const U&)):
            Reverse order of three_way_comparable_with template arguments.
            * testsuite/20_util/optional/relops/104606.cc: New test.

    (cherry picked from commit 7f65d8267fbfd19cf21a3dc71d27e989e75044a3)

[Bug libstdc++/104606] [11/12 Regression] comparison operator resolution with std::optional and -std=c++20

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |13.2.1, 14.0
            Summary|[11/12/13 Regression]       |[11/12 Regression]
                   |comparison operator         |comparison operator
                   |resolution with             |resolution with
                   |std::optional and           |std::optional and
                   |-std=c++20                  |-std=c++20

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 13.3 and 14.1 so far.