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

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

[Marek Polacek]

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_cadidate 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?

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-08-29  Marek Polacek  <polacek@redhat.com>

	PR c++/87109
	* call.c (add_function_candidate): Skip an rvalue ref-qualified
	candidate function when performing implicit move semantics.

	* g++.dg/cpp0x/ref-qual19.C: New test.

diff --git gcc/cp/call.c gcc/cp/call.c
index a1567026975..2911b4cfdaa 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -2194,6 +2194,7 @@ add_function_candidate (struct z_candidate **candidates,
       is_this = (i == 0 && DECL_NONSTATIC_MEMBER_FUNCTION_P (fn)
 		 && ! DECL_CONSTRUCTOR_P (fn));
 
+      bool iop_to_rvalue = false;
       if (parmnode)
 	{
 	  tree parmtype = TREE_VALUE (parmnode);
@@ -2222,6 +2223,7 @@ add_function_candidate (struct z_candidate **candidates,
 		  /* The special handling of 'this' conversions in compare_ics
 		     does not apply if there is a ref-qualifier.  */
 		  is_this = false;
+		  iop_to_rvalue = rv;
 		}
 	      else
 		{
@@ -2247,6 +2249,14 @@ add_function_candidate (struct z_candidate **candidates,
 	  to_type = argtype;
 	}
 
+      /* We are performing implicit move, so the argument is an xvalue.
+	 If this candidate is an rvalue ref-qualified function, it's
+	 turned its parameter to an rvalue reference, in which case this
+	 function would be a viable candidate.  But it seems that's not
+	 what we want.  */
+      if (t && iop_to_rvalue && (flags & LOOKUP_PREFER_RVALUE))
+	t->bad_p = true;
+
       if (t && is_this)
 	t->this_p = true;
 
diff --git gcc/testsuite/g++.dg/cpp0x/ref-qual19.C gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
index e69de29bb2d..b34007cd57d 100644
--- gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
+++ gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
@@ -0,0 +1,42 @@
+// PR c++/87109
+// { dg-do run { target c++11 } }
+
+#include <utility>
+
+struct C { int i; };
+struct A {
+  operator C() & { return { 1 }; }
+  operator C() && { return { 2 }; }
+};
+
+C
+f (A a)
+{
+  return a;
+}
+
+C
+f2 (A a)
+{
+  return std::move (a);
+}
+
+C
+f3 ()
+{
+  return A();
+}
+
+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 != 2)
+    __builtin_abort ();
+}

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

[Jason Merrill]

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.

Jason

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

[Marek Polacek]

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?

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-09-04  Marek Polacek  <polacek@redhat.com>

	PR c++/87109
	* call.c (build_user_type_conversion_1): If LOOKUP_PREFER_RVALUE, set
	rvaluedness_matches_p on ck_rvalue.
	(source_expr): New.
	(build_over_call): Check if the returned object's type is the same as
	the argument this ctor received.

	* g++.dg/cpp0x/ref-qual19.C: New test.

diff --git gcc/cp/call.c gcc/cp/call.c
index a1567026975..6f998e1201a 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -3919,7 +3919,11 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
 	     harmless since we'd add it here anyway. */
 	  if (ics && MAYBE_CLASS_TYPE_P (totype) && ics->kind != ck_rvalue
 	      && !(convflags & LOOKUP_NO_TEMP_BIND))
-	    ics = build_conv (ck_rvalue, totype, ics);
+	    {
+	      ics = build_conv (ck_rvalue, totype, ics);
+	      if (flags & LOOKUP_PREFER_RVALUE)
+		ics->rvaluedness_matches_p = true;
+	    }
 
 	  cand->second_conv = ics;
 
@@ -7722,6 +7726,16 @@ build_trivial_dtor_call (tree instance)
 		 instance, clobber);
 }
 
+/* The source expr for this standard conversion sequence.  */
+
+static tree
+source_expr (conversion *t)
+{
+  while (t->kind != ck_identity)
+    t = next_conversion (t);
+  return t->u.expr;
+}
+
 /* Subroutine of the various build_*_call functions.  Overload resolution
    has chosen a winning candidate CAND; build up a CALL_EXPR accordingly.
    ARGS is a TREE_LIST of the unconverted arguments to the call.  FLAGS is a
@@ -7934,6 +7948,25 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
 	      || !TYPE_REF_IS_RVALUE (ptype)
 	      || CONVERSION_RANK (convs[0]) > cr_exact)
 	    return error_mark_node;
+
+	  ptype = non_reference (convs[0]->type);
+
+	  /* Find what we're converting from.  */
+	  tree t = source_expr (convs[0]);
+	  if (TREE_CODE (t) == TARGET_EXPR)
+	    t = TARGET_EXPR_INITIAL (t);
+	  tree fndecl = cp_get_callee_fndecl_nofold (t);
+	  /* We know now that the ctor takes an rvalue reference, now
+	     check that its object has the same as the returned object's
+	     type.  This assumes that the types may differ only if a
+	     user-defined conversion is in play.  */
+	  if (fndecl && DECL_CONV_FN_P (fndecl))
+	    {
+	      t = DECL_ARGUMENTS (fndecl);
+	      t = TREE_TYPE (TREE_TYPE (t));
+	      if (!same_type_ignoring_top_level_qualifiers_p (t, ptype))
+		return error_mark_node;
+	    }
 	}
     }
   /* Bypass access control for 'this' parameter.  */
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 ();
+}

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

[Jason Merrill]

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.

Jason

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

[Marek Polacek]

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 ();
+}

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

[Jason Merrill]

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 ();
> +}