[Bug c++/113074] New: requires requires should be SFINAE

[Bug c++/113074] New: requires requires should be SFINAE

[jdapena at igalia dot com]

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

            Bug ID: 113074
           Summary: requires requires should be SFINAE
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jdapena at igalia dot com
  Target Milestone: ---

Created attachment 56904
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56904&action=edit
Test case from godbolt

Calling std::to_address in a requires requires function should be SFINAE.
Instead of it, it fails to compile.

This is breaking GCC build of Chromium in
https://chromium.googlesource.com/chromium/src.git/+/refs/tags/122.0.6193.2/mojo/public/cpp/bindings/type_converter.h#98
when the type does not support std::to_address because it fails to find
operator->.

Code is:
template <typename T, typename U>
  requires requires(const U& obj) {
    not std::is_pointer_v<U>;
    { mojo::ConvertTo<T>(std::to_address(obj)) } -> std::same_as<T>;
  }
inline T ConvertTo(const U& obj) {
  return mojo::ConvertTo<T>(std::to_address(obj));
}

The code is designed to only declare ConvertTo in case it can convert from
std::to_address result.

A possible test case is attached. This case fails in Clang with the expected
outcome (it fails to resolve to a valid call).

[Bug libstdc++/113074] requires requires should be SFINAE

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Oh
https://cplusplus.github.io/LWG/issue3545

[Bug libstdc++/113074] requires requires should be SFINAE

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
> Oh
> https://cplusplus.github.io/LWG/issue3545

Whoops wrong one.

[Bug libstdc++/113074] requires requires should be SFINAE

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://eel.is/c++draft/memory#pointer.conversion


As far as I can tell, std::to_address is not required to be SFINAE safe ...

The standard just says:
```
Returns: pointer_traits<Ptr>​::​to_address(p) if that expression is well-formed
(see [pointer.traits.optmem]), otherwise to_address(p.operator->()).
```

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>This case fails in Clang with the expected outcome (it fails to resolve to a valid call).

No it fails the same way if you use libstdc++ from GCC.
It fails that way if you use libc++ from the LLVM project.

As far as I read the standard std::to_address does not have to be SFINAE  safe.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 56905
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56905&action=edit
testcase which shows libc++ and libstdc++ difference

with libstdc++, both GCC and clang reject this.
with libc++, clang accepts this (I only tested clang because it was the easiest
to test there).

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[de34 at live dot cn]

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

Jiang An <de34 at live dot cn> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |de34 at live dot cn

--- Comment #6 from Jiang An <de34 at live dot cn> ---
Currently only libc++'s std::to_address is SFINAE-friendly. I think we should
submit an LWG issue, although it would be possibly closed as NAD.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |oh my

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Just because something compiles with clang and/or libc++ doesn't mean gcc is
wrong, that's not how a standard works.

When this was brought up with LWG it was pointed out that the library only
checks for to_address when an iterator has explicitly opted in to it by using
iterator_concept = contiguous_iterator_concept and that it is better to error
than silently degrade to random access.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[pkasting at google dot com]

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

Peter Kasting <pkasting at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pkasting at google dot com

--- Comment #8 from Peter Kasting <pkasting at google dot com> ---
As a Chromium C++ OWNER -- agree that libstdc++ is standards-compliant as-is.
We'll fix our code.

I do think libc++'s behavior is more usable and an LWG issue would be nice.
There's currently no simple and blessed route for consumers to convert
raw-or-fancy-pointer input types to raw pointers.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #5)
> Created attachment 56905 [details]
> testcase which shows libc++ and libstdc++ difference
> 
> with libstdc++, both GCC and clang reject this.
> with libc++, clang accepts this (I only tested clang because it was the
> easiest to test there).

AFAICT only libc++ from llvm trunk accepts this, even 17.0.1 gives an error
outside the immediate context, like libstdc++ does.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[pkasting at google dot com]

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

--- Comment #10 from Peter Kasting <pkasting at google dot com> ---
(In reply to Jonathan Wakely from comment #9)
> (In reply to Andrew Pinski from comment #5)
> > Created attachment 56905 [details]
> > testcase which shows libc++ and libstdc++ difference
> > 
> > with libstdc++, both GCC and clang reject this.
> > with libc++, clang accepts this (I only tested clang because it was the
> > easiest to test there).
> 
> AFAICT only libc++ from llvm trunk accepts this, even 17.0.1 gives an error
> outside the immediate context, like libstdc++ does.

Yes, but that was because libc++ did not implement LWG3545 until LLVM 18.

When Jose reported this issue in Chromium I pushed back on fixing on our side
because I thought libstdc++ had the same issue. But this is a distinct issue;
std::to_address<> is not required to be SFINAE-compatible even if
std::pointer_traits<> now is.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Peter Kasting from comment #8)
> There's currently no simple and blessed route for consumers to convert
> raw-or-fancy-pointer input types to raw pointers.

I disagree, std::to_address does exactly that.

The problem here seems to be that Chromium is expecting it to do more than
that.

Given a raw-or-fancy-pointer std::to_address will turn it into a raw pointer.
What the testcase above seems to be trying to do is to use std::to_address as a
proxy for answering the question "is this a raw or fancy pointer". That's not
what it's for.

Can't you use something like this to check that instead?

template<typename T>
concept IsPointerLike = requires { typename std::pointer_traits<T>::pointer; }
                         || requires (const T& t) { t.operator->(); };

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Peter Kasting from comment #10)
> When Jose reported this issue in Chromium I pushed back on fixing on our
> side because I thought libstdc++ had the same issue. But this is a distinct
> issue; std::to_address<> is not required to be SFINAE-compatible even if
> std::pointer_traits<> now is.

Right, LWG 3545 did not touch std::to_address.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It looks like libc++ did it for this reason:

[libc++] Fix a hard error in `contiguous_iterator<NoOperatorArrowIter>`.

Evaluating `contiguous_iterator` on an iterator that satisfies all the
constraints except the `to_address` constraint and doesn't have
`operator->` defined results in a hard error. This is because
instantiating `to_address` ends up instantiating templates
dependent on the given type which might lead to a hard error even
in a SFINAE context.

Differential Revision: https://reviews.llvm.org/D130835


As I said in comment 7, LWG considered this case and it was pointed out that
the problem described can only occur if a type defines iterator_concept =
contiguous_iterator; but then fails to actually provide the operations needed
for a contiguous iterator (i.e. either a pointer_traits specialization with
to_address or a sane operator->()).

A SFINAE-friendly std::to_address as implemented in libc++ means that such an
iterator will fail to satisfy std::contiguous_iterator and will silently
degrade to satosfying std::random_access_iterator only. It's not at all clear
to me that silently degrading such an iterator (which very explicitly claims to
be a contiguous iterator by defining iterator_concept to say so) would be an
improvement. I'd rather get an error telling me the thing I thought was a
contiguous iterator was not actually.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[pkasting at google dot com]

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

--- Comment #14 from Peter Kasting <pkasting at google dot com> ---
(In reply to Jonathan Wakely from comment #13)
> As I said in comment 7, LWG considered this case and it was pointed out that
> the problem described can only occur if a type defines iterator_concept =
> contiguous_iterator; but then fails to actually provide the operations
> needed for a contiguous iterator (i.e. either a pointer_traits
> specialization with to_address or a sane operator->()).

That's fair. But that only considers the functionality of to_address() inside
this specific library use of it. If this tool is to be usable in other contexts
(which I argue it should be, or it shouldn't have been exposed to end users),
then said contexts may have nothing to do with iterators.

And you are right, it's possible to reimplement concepts around "is this even
legal to pass to to_address()", which is basically what we're doing to address
this in Chromium. But that's pretty unfriendly to most usage; if you're in a
context where you are reaching for to_address() to begin with, it's likely
because you're templated and don't know that a simpler thing like `&*ptr` is
valid. In such cases, having to_address() be SFINAE-friendly makes it far
easier to fall back to other handling for "not a pointer".

> A SFINAE-friendly std::to_address as implemented in libc++ means that such
> an iterator will fail to satisfy std::contiguous_iterator and will silently
> degrade to satosfying std::random_access_iterator only. It's not at all
> clear to me that silently degrading such an iterator (which very explicitly
> claims to be a contiguous iterator by defining iterator_concept to say so)
> would be an improvement. I'd rather get an error telling me the thing I
> thought was a contiguous iterator was not actually.

That's fair, but isn't that implementable by simply making the definition of
contiguous_iterator explicitly hard error in this case? That is, an
SFINAE-friendly to_address() wouldn't prevent you from diagnosing this
particular usage site as incorrect.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #15 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Peter Kasting from comment #14)
> And you are right, it's possible to reimplement concepts around "is this
> even legal to pass to to_address()", which is basically what we're doing to
> address this in Chromium. But that's pretty unfriendly to most usage; if
> you're in a context where you are reaching for to_address() to begin with,
> it's likely because you're templated and don't know that a simpler thing
> like `&*ptr` is valid.

The reason to avoid &*ptr is because it's undefined behaviour on a past-the-end
iterator, not because it might be ill-formed for some template argument types.

> That's fair, but isn't that implementable by simply making the definition of
> contiguous_iterator explicitly hard error in this case? That is, an
> SFINAE-friendly to_address() wouldn't prevent you from diagnosing this
> particular usage site as incorrect.

The rationale for making it SFINAE-friendly in libc++ was to *not* give an
error for that case. Making it SFINAE-friendly but restoring the error in that
case would remove the reason to have made it SFINAE-friendly in the first
place.

If you think this should change, please take that through WG21. I don't think
we should deviate from the standard here.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #15)
> The reason to avoid &*ptr is because it's undefined behaviour on a
> past-the-end iterator, not because it might be ill-formed for some template
> argument types.

Hmm, actually void* is the one type where &*ptr isn't valid but
std::to_address(ptr) still works. I don't think it's a common case though.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[pkasting at google dot com]

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

--- Comment #17 from Peter Kasting <pkasting at google dot com> ---
(In reply to Jonathan Wakely from comment #15)
> (In reply to Peter Kasting from comment #14)
> > And you are right, it's possible to reimplement concepts around "is this
> > even legal to pass to to_address()", which is basically what we're doing to
> > address this in Chromium. But that's pretty unfriendly to most usage; if
> > you're in a context where you are reaching for to_address() to begin with,
> > it's likely because you're templated and don't know that a simpler thing
> > like `&*ptr` is valid.
> 
> The reason to avoid &*ptr is because it's undefined behaviour on a
> past-the-end iterator, not because it might be ill-formed for some template
> argument types.

That's the reason to avoid it in the specific case the library happened to make
use of std::to_address(). That's not necessarily reasoning that applies to
other uses of std::to_address().

> > That's fair, but isn't that implementable by simply making the definition of
> > contiguous_iterator explicitly hard error in this case? That is, an
> > SFINAE-friendly to_address() wouldn't prevent you from diagnosing this
> > particular usage site as incorrect.
> 
> The rationale for making it SFINAE-friendly in libc++ was to *not* give an
> error for that case. Making it SFINAE-friendly but restoring the error in
> that case would remove the reason to have made it SFINAE-friendly in the
> first place.

I'm not sure how many more ways I can say that my interest in this being
SFINAE-friendly is completely unrelated to libc++'s (or any other STL's)
handling of contiguous_iterator.

The committee added a tool, used it one place, made it hard to hold other
places, and (per your previous summaries) has avoided making it more useful
other places due to only thinking about things through the lens of this
specific contiguous_iterator usage. I'm not using it for that.

> If you think this should change, please take that through WG21. I don't
> think we should deviate from the standard here.

Yes, precisely. Per comment 8: "libstdc++ is standards-compliant as-is. We'll
fix our code. I do think libc++'s behavior is more usable and an LWG issue
would be nice."

I don't have any connection to WG21. I was hoping someone here had the
experience necessary to pass on this request to the committee. If not, so be
it, it will die.

[Bug libstdc++/113074] std::to_address should be SFINAE safe

[redi at gcc dot gnu.org]

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

--- Comment #18 from Jonathan Wakely <redi at gcc dot gnu.org> ---
See http://cplusplus.github.io/LWG/lwg-active.html#submit_issue