[Bug libstdc++/107525] New: propagate_const should not be using SFINAE on its conversion operators

[Bug libstdc++/107525] New: propagate_const should not be using SFINAE on its conversion operators

[dangelog at gmail dot com]

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

            Bug ID: 107525
           Summary: propagate_const should not be using SFINAE on its
                    conversion operators
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dangelog at gmail dot com
  Target Milestone: ---

propagate_const in the LFTSv3 has implicit conversion operators which have
constraints on them:

https://cplusplus.github.io/fundamentals-ts/v3.html#propagate_const.const_observers

> constexpr operator const element_type*() const;
> 
> Constraints:
>    T is an object pointer type or has an implicit conversion to const element_type*. 
> Returns:
>    get().

libstdc++ implements these constraints by means of SFINAE on the operators.
This is user-hostile: using SFINAE means that the conversion operator is now a
function template, and that means that https://eel.is/c++draft/over.ics.user#3
kicks in:

> If the user-defined conversion is specified by a specialization of a conversion function template, the second standard conversion sequence shall have exact match rank.


Concretely, this means that for instance we lose implicit conversions towards
base classes of the pointed-to type:

    std::experimental::propagate_const<Derived *> ptr;

    Derived *d1 = ptr;  // Convert precisely to Derived *: OK
    Base *b1    = ptr;  // Convert to a pointer to a base: ERROR

    Base *b2    = static_cast<Derived *>(ptr); // OK
    Base *b3    = static_cast<Base *>(ptr);    // ERROR

    Base *b4    = ptr.get();  // OK


But these should all work. The design of propagate_const is for it to be
"drop-in replacement", maximizing compatibility with existing code.
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4388.html says
explictly

"When T is an object pointer type operator value* exists and allows implicit
conversion to a pointer. This avoids using get to access the pointer in
contexts where it was unnecesary before addition of the propagate_const
wrapper."

--

So. ideally, the conversion operators should be using C++20 constraints, but of
course that's not possible. I guess that a reasonable alternative would be to
isolate them in a base class, and apply SFINAE on that base class instead?

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Keywords|                            |rejects-valid
   Last reconfirmed|                            |2022-11-04
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Giuseppe D'Angelo from comment #0)
> So. ideally, the conversion operators should be using C++20 constraints, but
> of course that's not possible.

It's totally possible for C++20 mode.

I don't know how much motivation anybody has to do anything about this though.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Also, the constraint is wrong for the non-const conversion. It checks for
convertibility to const element_type* but should be element_type*.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
And the spec seems wrong as well. The const overload should be constrained for
const T being convertible to const element_type*.

This should probably not compile:

#include <experimental/propagate_const>

struct X
{
  int& operator*() { return *i; }
  const int& operator*() const { return *i; }
  int* get() { return i; }
  const int* get() const { return i; }
  int* i = nullptr;

  operator int*() { return i; }
  operator const int*() const = delete;
};

int main()
{
  using std::experimental::propagate_const;

  X x;
  const propagate_const<X> px(x);

  static_assert(!std::is_convertible_v<const X, const int*>);
  static_assert(std::is_convertible_v<const propagate_const<X>, const int*>);
  const int* pi = px;
}

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[dangelog at gmail dot com]

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

--- Comment #4 from Giuseppe D'Angelo <dangelog at gmail dot com> ---
(In reply to Jonathan Wakely from comment #1)
> (In reply to Giuseppe D'Angelo from comment #0)
> > So. ideally, the conversion operators should be using C++20 constraints, but
> > of course that's not possible.
> 
> It's totally possible for C++20 mode.
> 
> I don't know how much motivation anybody has to do anything about this
> though.

Sorry, what I meant is, of course there is interest at keeping this code to
compile in pre-C++20 mode, and possibly have the same semantics no matter
what's the language version used? Or is it acceptable to have such an "API
break"? (E.g. stuff like `is_convertible_v<propagate_const<Derived *>, Base *>`
changes value.)


> And the spec seems wrong as well. The const overload should be constrained for const T being convertible to const element_type*.

Yes, that sounds like a defect to me.

--

More in general, I think these operators are strangely defined. I'm not sure
why they're not simply defined to be 

  template <typename U> operator U *() requires (std::is_convertible_v<T, U
*>);

mut.mut. for the `const` version. 

The current definition also allows for pointer arithmetic (only if one uses a
C++20 constraint, otherwise it doesn't work), which is something the original
paper says it does NOT want to support. And the current definition allows for
`delete`ing a propagate_const, which maybe is wanted, but in contradiction with
the lack of support for pointer arithmetic.

I guess I'll need to submit a paper.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Giuseppe D'Angelo from comment #4)
> Sorry, what I meant is, of course there is interest at keeping this code to
> compile in pre-C++20 mode, and possibly have the same semantics no matter
> what's the language version used? Or is it acceptable to have such an "API
> break"? (E.g. stuff like `is_convertible_v<propagate_const<Derived *>, Base
> *>` changes value.)

I think that's fine for TS material.


> More in general, I think these operators are strangely defined. I'm not sure
> why they're not simply defined to be 
> 
>   template <typename U> operator U *() requires (std::is_convertible_v<T, U
> *>);
> 
> mut.mut. for the `const` version. 

That seems better to me.

> The current definition also allows for pointer arithmetic (only if one uses
> a C++20 constraint, otherwise it doesn't work), which is something the
> original paper says it does NOT want to support. And the current definition
> allows for `delete`ing a propagate_const, which maybe is wanted, but in
> contradiction with the lack of support for pointer arithmetic.
> 
> I guess I'll need to submit a paper.

Please don't! At least not in the next 9 days. We intend to vote out LFTSv3 at
next week's meeting, there will be no more proposals for LFTS considered. And I
don't want to derail the vote next week by having open proposals arriving at
the last minute.

Feel free to file a library issue though. If propagate_const ever gets proposed
for inclusion in the standard then this should be taken into account.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

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

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Created attachment 53825
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53825&action=edit
Do not use SFINAE for propagate_const conversions

I got nerd sniped by the incorrect constraint, so replaced them with base
classes as you suggested. I'll probably push this next week.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

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

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Ping Ville: please take a look

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[ville.voutilainen at gmail dot com]

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

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

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

--- Comment #8 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
(In reply to Jonathan Wakely from comment #7)
> Ping Ville: please take a look

Looks fine to me.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[dangelog at gmail dot com]

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

--- Comment #9 from Giuseppe D'Angelo <dangelog at gmail dot com> ---
(In reply to Jonathan Wakely from comment #5) 
> Please don't! At least not in the next 9 days. We intend to vote out LFTSv3
> at next week's meeting, there will be no more proposals for LFTS considered.
> And I don't want to derail the vote next week by having open proposals
> arriving at the last minute.

No problem, I wouldn't have done this before a few weeks anyhow.


> Feel free to file a library issue though. If propagate_const ever gets
> proposed for inclusion in the standard then this should be taken into
> account.

OK.

Thank you very much for the prompt fix.

(Ville: guess what, this bug stem from trying to make uic and friends use
propagate_const. Small world!)

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[cvs-commit at gcc dot gnu.org]

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

--- 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:7c6008e75df80607f8104e665e0448a0a9cbf85a

commit r13-3695-g7c6008e75df80607f8104e665e0448a0a9cbf85a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Nov 4 15:05:41 2022 +0000

    libstdc++: Do not use SFINAE for propagate_const conversions [PR107525]

    As the PR notes, the current conversion operators are defined as
    function templates so that we can use SFINAE. But this changes how they
    are considered for overload resolution. This moves those operators into
    base classes that can be specialized so the operators are obsent unless
    the constraints are satisfied.

    libstdc++-v3/ChangeLog:

            PR libstdc++/107525
            * include/experimental/propagate_const (operator element_type*()):
            Move into base class that can be partially specilized to iompose
            constraints.
            (operator const element_type*()): Likewise.
            * testsuite/experimental/propagate_const/observers/107525.cc: New
test.

[Bug libstdc++/107525] propagate_const should not be using SFINAE on its conversion operators

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |13.0

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk, thanks for the report (and the library issue submission).