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

[Marek Polacek <polacek@redhat.com>]

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