Re: C++ PATCH/RFC for c++/87109, wrong overload with ref-qualifiers

[Jason Merrill <jason@redhat.com>]

OK, thanks.

On Wed, Sep 5, 2018 at 4:09 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Sep 04, 2018 at 05:29:00PM -0400, Jason Merrill wrote:
>> On Tue, Sep 4, 2018 at 3:02 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Thu, Aug 30, 2018 at 09:22:26AM -0400, Jason Merrill wrote:
>> >> On Wed, Aug 29, 2018 at 8:03 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >> > I've now gotten to the point where I question the validity of this PR, so it's
>> >> > probably a good time to stop and ask for some advice.
>> >> >
>> >> > As discussed in <https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01607.html>, we
>> >> > choose the wrong overload for f1:
>> >> >
>> >> > struct C { };
>> >> > struct A {
>> >> >   operator C() &;
>> >> >   operator C() &&;
>> >> > };
>> >> >
>> >> > C f1(A a)
>> >> > {
>> >> >    return a; // should call operator C()&, but calls operator C()&&
>> >> > }
>> >> >
>> >> > Since we're returning a local variable, we know it's about to be destroyed,
>> >> > so even though it's got a name, not for very much longer, so we activate move
>> >> > semantics.  So we perform overload resolution with 'a' turned into
>> >> > *NON_LVALUE_EXPR<(A&) &a>, an xvalue.  We need to convert 'a' from A to C,
>> >> > which is taking place in build_user_type_conversion_1.  It will see two
>> >> > cadidates:
>> >> >
>> >> >   A::operator C() &&
>> >> >   A::operator C() &
>> >> >
>> >> > when adding these candidates in add_function_candidate we process the
>> >> > ref-qualifiers by tweaking the type of the implicit object parameter by turning
>> >> > it into a reference type.  Then we create an implicit conversion sequence
>> >> > for converting the type of the argument to the type of the parameter,
>> >> > so A to A&.  That succeeds in the first case (an xvalue binding to an rvalue
>> >> > reference) but fails in the second case (an xvalue binding to an lvalue
>> >> > reference).  And thus we end up using the first overload.
>> >> >
>> >> > But why is this invalid, again?  [class.copy.elision] says "or if the type of
>> >> > the first parameter of the selected constructor is not an rvalue reference to
>> >> > the object's type (possibly cv-qualified), overload resolution is performed
>> >> > again, considering the object as an lvalue." but I don't see how that applies
>> >> > here.  (Constructors can't have ref-qualifiers anyway.)
>> >> >
>> >> > Thoughts?
>> >>
>> >> Where that rule comes in is when we choose the constructor for C:
>> >> since we've already called operator C()&&, we choose C(C&&), which
>> >> does not have a first parameter of "rvalue reference to cv A", so it
>> >> should be rejected.
>> >
>> > I see.  Turned out there were two problems: we weren't getting into the
>> > if (flags & LOOKUP_PREFER_RVALUE) block in build_over_call; this I fixed
>> > by setting rvaluedness_matches_p in build_user_type_conversion_1 for ck_rvalue
>> > if appropriate.
>> >
>> > And then the constructor argument check failed to check the returned object's
>> > type.  The tricky part is getting the type.  I realized we can go to the
>> > beginning of the conversion sequence and extract u.expr.  But we can't simply
>> > look at its type, if it's DECL_CONV_FN_P, we need to dig deeper.  You'll see
>> > that I don't handle other things, and I don't know if I need to.  I tried to
>> > handle arbitrary expressions, but it evolved into something that just seemed
>> > too fragile.
>> >
>> > E.g. for this testcase u.expr will look like:
>> > TARGET_EXPR <D.2335, A::operator C ((struct A *) NON_LVALUE_EXPR <(struct A &) &a>)>
>> > and taking its type would yield "struct C".  I actually think that returning
>> > error_mark_node when we see any DECL_CONV_FN_P would work just as well.
>> >
>> > Can you think of something better than this?
>>
>> Hmm, I think we could push that back even farther, and bail out where
>> you're already changing build_user_type_conversion_1; if we're doing
>> this two-step initialization, then we aren't going to end up with a
>> suitable constructor.
>
> That makes sense.  There was one snag: we don't want to make a candidate
> non-viable only because of LOOKUP_PREFER_RVALUE before the checking if
> the conversion was ambiguous: if we have two candidates, then killing one
> of them would make the conversion non-ambiguous (as in template/spec22.C).
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-09-05  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/87109, wrong overload with ref-qualifiers.
>         * call.c (build_user_type_conversion_1): Use NULL instead of 0.  Bail
>         out if performing the maybe-rvalue overload resolution and a conversion
>         function is getting called.
>
>         * g++.dg/cpp0x/ref-qual19.C: New test.
>
> diff --git gcc/cp/call.c gcc/cp/call.c
> index a1567026975..942b2c204be 100644
> --- gcc/cp/call.c
> +++ gcc/cp/call.c
> @@ -3971,7 +3971,7 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
>      }
>
>    cand = tourney (candidates, complain);
> -  if (cand == 0)
> +  if (cand == NULL)
>      {
>        if (complain & tf_error)
>         {
> @@ -4014,6 +4014,12 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
>    if (cand->viable == -1)
>      conv->bad_p = true;
>
> +  /* We're performing the maybe-rvalue overload resolution and
> +     a conversion function is in play.  This isn't going to work
> +     because we would not end up with a suitable constructor.  */
> +  if ((flags & LOOKUP_PREFER_RVALUE) && !DECL_CONSTRUCTOR_P (cand->fn))
> +    return NULL;
> +
>    /* Remember that this was a list-initialization.  */
>    if (flags & LOOKUP_NO_NARROWING)
>      conv->check_narrowing = true;
> diff --git gcc/testsuite/g++.dg/cpp0x/ref-qual19.C gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
> index e69de29bb2d..8494b83e5b0 100644
> --- gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
> +++ gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
> @@ -0,0 +1,117 @@
> +// PR c++/87109
> +// { dg-do run { target c++11 } }
> +
> +#include <utility>
> +
> +struct C { int i; };
> +struct D { int i; };
> +
> +struct A {
> +  int j;
> +  operator C() & { return { 1 }; }
> +  operator C() && { return { 2 }; }
> +};
> +
> +struct B : public A {
> +  operator D() & { return { 3 }; }
> +  operator D() && { return { 4 }; }
> +};
> +
> +C
> +f (A a)
> +{
> +  return a;
> +}
> +
> +C
> +f2 (A a)
> +{
> +  return std::move (a);
> +}
> +
> +C
> +f3 ()
> +{
> +  A a;
> +  return a;
> +}
> +
> +C
> +f4 ()
> +{
> +  A a;
> +  return std::move (a);
> +}
> +
> +C
> +f5 ()
> +{
> +  return A();
> +}
> +
> +D
> +f6 (B b)
> +{
> +  return b;
> +}
> +
> +D
> +f7 (B b)
> +{
> +  return std::move (b);
> +}
> +
> +D
> +f8 ()
> +{
> +  B b;
> +  return b;
> +}
> +
> +D
> +f9 ()
> +{
> +  B b;
> +  return std::move (b);
> +}
> +
> +D
> +f10 ()
> +{
> +  return B();
> +}
> +
> +int
> +main ()
> +{
> +  C c1 = f (A());
> +  if (c1.i != 1)
> +    __builtin_abort ();
> +  C c2 = f2 (A());
> +  if (c2.i != 2)
> +    __builtin_abort ();
> +  C c3 = f3 ();
> +  if (c3.i != 1)
> +    __builtin_abort ();
> +  C c4 = f4 ();
> +  if (c4.i != 2)
> +    __builtin_abort ();
> +  C c5 = f5 ();
> +  if (c5.i != 2)
> +    __builtin_abort ();
> +  D c6 = f6 (B());
> +  if (c6.i != 3)
> +    __builtin_abort ();
> +  D c7 = f7 (B());
> +  if (c7.i != 4)
> +    __builtin_abort ();
> +  D c8 = f8 ();
> +  if (c8.i != 3)
> +    __builtin_abort ();
> +  D c9 = f9 ();
> +  if (c9.i != 4)
> +    __builtin_abort ();
> +  D c10 = f10 ();
> +  if (c10.i != 4)
> +    __builtin_abort ();
> +}