C++ PATCH for c++/25950 (DR391, binding references to temporaries)

[Jason Merrill <jason at redhat dot com>]

PR c++/25950 requests that we implement DR391; this is the most commonly 
reported issue in the C++ compiler, so it seemed worth fixing to me. 
Basically, people want to be able to bind a temporary of a class with an 
uncallable copy constructor to a const reference; C++98 doesn't allow 
this, but DR391 says that was a bug in the standard, and requires 
compilers to always bind references directly to temporaries of 
compatible class type.

When the rvalue references patch went in, DR391 was partially 
implemented, but only for C++0x mode.  This seems wrong to me; if the 
committee has decided that the standard was wrong, it seems perverse for 
us to cling to the letter of the standard when users are clamoring for 
us to change our behavior.  C++0x mode makes some sense for controlling 
new language features, but not for DRs.

The implementation that went in for C++0x mode broke 
g++.dg/overload/arg[14].C:

  struct A {};
  struct B : A
  {
    B(int);
    B(B &);
    B(A);
  };

  void foo(B);

  void bar()
  {
    foo(0); // { dg-error "" }
  }

It allowed us to call foo(B(A(B(0))), allowing the call to succeed. 
Previously we rejected this by requiring that the B copy constructor be 
callable even if we don't use it, resulting in an odd error message that 
complains about being unable to find a copy constructor for B when 
trying to pass an A argument.

I've fixed this testcase differently, by treating the derived-to-base 
conversion as a user-defined conversion since it involves calling a 
constructor, even though it has standard conversion rank.  A conversion 
sequence can only have one user-defined conversion, so the conversion to 
A is not considered.  We still allow the derived-to-base conversion when 
it is really only a pointer conversion.  This requires a somewhat subtle 
reading of the standard, but I believe it to be the correct solution.

Nathan: in the comments for PR 17431 where this testcase came up, you 
mention that EDG pointed out to you how the testcase is invalid, but you 
didn't include the actual information in the comment.  Do you remember 
if their analysis was anything like my analysis above?

Tested x86_64-pc-linux-gnu, applied to trunk.
2007-10-23  Jason Merrill  <jason@redhat.com>

	PR c++/25950 (DR 391)
	* call.c (struct conversion): Remove check_copy_constructor_p.
	(reference_binding): Always bind a reference directly to a 
	compatible class rvalue.  Pass down LOOKUP_NO_TEMP_BIND during 
	temporary creation.
	(check_constructor_callable): Remove.
	(convert_like_real): Don't call it.
	(initialize_reference): Don't call check_constructor_callable.
	(standard_conversion): Check LOOKUP_NO_CONVERSION instead of
	LOOKUP_CONSTRUCTOR_CALLABLE.  Don't require a temporary for base
	conversions if LOOKUP_NO_TEMP_BIND.
	(implicit_conversion): Pass through LOOKUP_NO_TEMP_BIND.
	(build_user_type_conversion_1): Pass through LOOKUP_NO_TEMP_BIND for
	second conversion.
	* cp-tree.h (LOOKUP_CONSTRUCTOR_CALLABLE): Remove.

Index: cp/call.c
===================================================================
*** cp/call.c	(revision 129215)
--- cp/call.c	(working copy)
*************** struct conversion {
*** 89,98 ****
       temporary should be created to hold the result of the
       conversion.  */
    BOOL_BITFIELD need_temporary_p : 1;
-   /* If KIND is ck_identity or ck_base_conv, true to indicate that the
-      copy constructor must be accessible, even though it is not being
-      used.  */
-   BOOL_BITFIELD check_copy_constructor_p : 1;
    /* If KIND is ck_ptr or ck_pmem, true to indicate that a conversion
       from a pointer-to-derived to pointer-to-base is being performed.  */
    BOOL_BITFIELD base_p : 1;
--- 89,94 ----
*************** static conversion *merge_conversion_sequ
*** 201,207 ****
  static bool magic_varargs_p (tree);
  typedef void (*diagnostic_fn_t) (const char *, ...) ATTRIBUTE_GCC_CXXDIAG(1,2);
  static tree build_temp (tree, tree, int, diagnostic_fn_t *);
- static void check_constructor_callable (tree, tree);
  
  /* Returns nonzero iff the destructor name specified in NAME matches BASETYPE.
     NAME can take many forms...  */
--- 197,202 ----
*************** standard_conversion (tree to, tree from,
*** 866,872 ****
    else if (fcode == VECTOR_TYPE && tcode == VECTOR_TYPE
  	   && vector_types_convertible_p (from, to, false))
      return build_conv (ck_std, to, conv);
!   else if (!(flags & LOOKUP_CONSTRUCTOR_CALLABLE)
  	   && IS_AGGR_TYPE (to) && IS_AGGR_TYPE (from)
  	   && is_properly_derived_from (from, to))
      {
--- 861,873 ----
    else if (fcode == VECTOR_TYPE && tcode == VECTOR_TYPE
  	   && vector_types_convertible_p (from, to, false))
      return build_conv (ck_std, to, conv);
!   /* A derived-to-base conversion sequence is a user-defined conversion
!      because it involves a constructor call, even though it has the rank of
!      a standard conversion, so we don't consider it if we aren't allowing
!      user-defined conversions.  But if we're binding directly to a
!      reference, it's only a pointer conversion.  */
!   else if ((!(flags & LOOKUP_NO_CONVERSION)
! 	    || (flags & LOOKUP_NO_TEMP_BIND))
  	   && IS_AGGR_TYPE (to) && IS_AGGR_TYPE (from)
  	   && is_properly_derived_from (from, to))
      {
*************** standard_conversion (tree to, tree from,
*** 876,883 ****
        /* The derived-to-base conversion indicates the initialization
  	 of a parameter with base type from an object of a derived
  	 type.  A temporary object is created to hold the result of
! 	 the conversion.  */
!       conv->need_temporary_p = true;
      }
    else
      return NULL;
--- 877,884 ----
        /* The derived-to-base conversion indicates the initialization
  	 of a parameter with base type from an object of a derived
  	 type.  A temporary object is created to hold the result of
! 	 the conversion unless we're binding directly to a reference.  */
!       conv->need_temporary_p = !(flags & LOOKUP_NO_TEMP_BIND);
      }
    else
      return NULL;
*************** reference_binding (tree rto, tree rfrom,
*** 1153,1166 ****
    compatible_p = reference_compatible_p (to, from);
  
    /* Directly bind reference when target expression's type is compatible with
!      the reference and expression is an lvalue. In C++0x, the wording in
!      [8.5.3/5 dcl.init.ref] is changed to also allow direct bindings for const
!      and rvalue references to rvalues of compatible class type, as part of
!      DR391. */
    if (compatible_p
        && (lvalue_p
! 	  || ((cxx_dialect != cxx98)
! 	      && (CP_TYPE_CONST_NON_VOLATILE_P(to) || TYPE_REF_IS_RVALUE (rto))
  	      && CLASS_TYPE_P (from))))
      {
        /* [dcl.init.ref]
--- 1154,1165 ----
    compatible_p = reference_compatible_p (to, from);
  
    /* Directly bind reference when target expression's type is compatible with
!      the reference and expression is an lvalue. In DR391, the wording in
!      [8.5.3/5 dcl.init.ref] is changed to also require direct bindings for
!      const and rvalue references to rvalues of compatible class type. */
    if (compatible_p
        && (lvalue_p
! 	  || ((CP_TYPE_CONST_NON_VOLATILE_P(to) || TYPE_REF_IS_RVALUE (rto))
  	      && CLASS_TYPE_P (from))))
      {
        /* [dcl.init.ref]
*************** reference_binding (tree rto, tree rfrom,
*** 1171,1177 ****
  	    is reference-compatible with "cv2 T2,"
  
  	 the reference is bound directly to the initializer expression
! 	 lvalue.  */
        conv = build_identity_conv (from, expr);
        conv = direct_reference_binding (rto, conv);
  
--- 1170,1183 ----
  	    is reference-compatible with "cv2 T2,"
  
  	 the reference is bound directly to the initializer expression
! 	 lvalue.
! 
! 	 [...]
! 	 If the initializer expression is an rvalue, with T2 a class type,
! 	 and "cv1 T1" is reference-compatible with "cv2 T2", the reference
! 	 is bound to the object represented by the rvalue or to a sub-object
! 	 within that object.  */
! 
        conv = build_identity_conv (from, expr);
        conv = direct_reference_binding (rto, conv);
  
*************** reference_binding (tree rto, tree rfrom,
*** 1251,1282 ****
  
    /* [dcl.init.ref]
  
-      If the initializer expression is an rvalue, with T2 a class type,
-      and "cv1 T1" is reference-compatible with "cv2 T2", the reference
-      is bound in one of the following ways:
- 
-      -- The reference is bound to the object represented by the rvalue
- 	or to a sub-object within that object.
- 
-      -- ...
- 
-      We use the first alternative.  The implicit conversion sequence
-      is supposed to be same as we would obtain by generating a
-      temporary.  Fortunately, if the types are reference compatible,
-      then this is either an identity conversion or the derived-to-base
-      conversion, just as for direct binding.  */
-   if (CLASS_TYPE_P (from) && compatible_p)
-     {
-       conv = build_identity_conv (from, expr);
-       conv = direct_reference_binding (rto, conv);
-       conv->rvaluedness_matches_p = TYPE_REF_IS_RVALUE (rto);
-       if (!(flags & LOOKUP_CONSTRUCTOR_CALLABLE))
- 	conv->u.next->check_copy_constructor_p = true;
-       return conv;
-     }
- 
-   /* [dcl.init.ref]
- 
       Otherwise, a temporary of type "cv1 T1" is created and
       initialized from the initializer expression using the rules for a
       non-reference copy initialization.  If T1 is reference-related to
--- 1257,1262 ----
*************** reference_binding (tree rto, tree rfrom,
*** 1285,1290 ****
--- 1265,1275 ----
    if (related_p && !at_least_as_qualified_p (to, from))
      return NULL;
  
+   /* We're generating a temporary now, but don't bind any more in the
+      conversion (specifically, don't slice the temporary returned by a
+      conversion operator).  */
+   flags |= LOOKUP_NO_TEMP_BIND;
+ 
    conv = implicit_conversion (to, from, expr, c_cast_p,
  			      flags);
    if (!conv)
*************** implicit_conversion (tree to, tree from,
*** 1329,1337 ****
        && (flags & LOOKUP_NO_CONVERSION) == 0)
      {
        struct z_candidate *cand;
  
!       cand = build_user_type_conversion_1
! 	(to, expr, LOOKUP_ONLYCONVERTING);
        if (cand)
  	conv = cand->second_conv;
  
--- 1314,1323 ----
        && (flags & LOOKUP_NO_CONVERSION) == 0)
      {
        struct z_candidate *cand;
+       int convflags = ((flags & LOOKUP_NO_TEMP_BIND)
+ 		       |LOOKUP_ONLYCONVERTING);
  
!       cand = build_user_type_conversion_1 (to, expr, convflags);
        if (cand)
  	conv = cand->second_conv;
  
*************** build_user_type_conversion_1 (tree totyp
*** 2590,2595 ****
--- 2576,2582 ----
    conversion *conv = NULL;
    tree args = NULL_TREE;
    bool any_viable_p;
+   int convflags;
  
    /* We represent conversion within a hierarchy using RVALUE_CONV and
       BASE_CONV, as specified by [over.best.ics]; these become plain
*************** build_user_type_conversion_1 (tree totyp
*** 2606,2611 ****
--- 2593,2603 ----
    candidates = 0;
    flags |= LOOKUP_NO_CONVERSION;
  
+   /* It's OK to bind a temporary for converting constructor arguments, but
+      not in converting the return value of a conversion operator.  */
+   convflags = ((flags & LOOKUP_NO_TEMP_BIND) | LOOKUP_NO_CONVERSION);
+   flags &= ~LOOKUP_NO_TEMP_BIND;
+ 
    if (ctors)
      {
        tree t;
*************** build_user_type_conversion_1 (tree totyp
*** 2650,2656 ****
      {
        tree fns;
        tree conversion_path = TREE_PURPOSE (conv_fns);
-       int convflags = LOOKUP_NO_CONVERSION;
  
        /* If we are called to convert to a reference type, we are trying to
  	 find an lvalue binding, so don't even consider temporaries.  If
--- 2642,2647 ----
*************** enforce_access (tree basetype_path, tree
*** 4256,4276 ****
    return true;
  }
  
- /* Check that a callable constructor to initialize a temporary of
-    TYPE from an EXPR exists.  */
- 
- static void
- check_constructor_callable (tree type, tree expr)
- {
-   build_special_member_call (NULL_TREE,
- 			     complete_ctor_identifier,
- 			     build_tree_list (NULL_TREE, expr),
- 			     type,
- 			     LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING
- 			     | LOOKUP_NO_CONVERSION
- 			     | LOOKUP_CONSTRUCTOR_CALLABLE);
- }
- 
  /* Initialize a temporary of type TYPE with EXPR.  The FLAGS are a
     bitwise or of LOOKUP_* values.  If any errors are warnings are
     generated, set *DIAGNOSTIC_FN to "error" or "warning",
--- 4247,4252 ----
*************** convert_like_real (conversion *convs, tr
*** 4428,4435 ****
  	 leave it as an lvalue.  */
        if (inner >= 0)
  	expr = decl_constant_value (expr);
-       if (convs->check_copy_constructor_p)
- 	check_constructor_callable (totype, expr);
        return expr;
      case ck_ambig:
        /* Call build_user_type_conversion again for the error.  */
--- 4404,4409 ----
*************** convert_like_real (conversion *convs, tr
*** 4459,4466 ****
  	{
  	  /* We are going to bind a reference directly to a base-class
  	     subobject of EXPR.  */
- 	  if (convs->check_copy_constructor_p)
- 	    check_constructor_callable (TREE_TYPE (expr), expr);
  	  /* Build an expression for `*((base*) &expr)'.  */
  	  expr = build_unary_op (ADDR_EXPR, expr, 0);
  	  expr = convert_to_base (expr, build_pointer_type (totype),
--- 4433,4438 ----
*************** initialize_reference (tree type, tree ex
*** 6782,6789 ****
  	 remember that the conversion was required.  */
        if (conv->kind == ck_base)
  	{
- 	  if (conv->check_copy_constructor_p)
- 	    check_constructor_callable (TREE_TYPE (expr), expr);
  	  base_conv_type = conv->type;
  	  conv = conv->u.next;
  	}
--- 6754,6759 ----
Index: cp/cp-tree.h
===================================================================
*** cp/cp-tree.h	(revision 129215)
--- cp/cp-tree.h	(working copy)
*************** enum overload_flags { NO_SPECIAL = 0, DT
*** 3699,3711 ****
  #define LOOKUP_PREFER_NAMESPACES (1 << 9)
  /* Accept types or namespaces.  */
  #define LOOKUP_PREFER_BOTH (LOOKUP_PREFER_TYPES | LOOKUP_PREFER_NAMESPACES)
- /* We are checking that a constructor can be called -- but we do not
-    actually plan to call it.  */
- #define LOOKUP_CONSTRUCTOR_CALLABLE (1 << 10)
  /* Return friend declarations and un-declared builtin functions.
     (Normally, these entities are registered in the symbol table, but
     not found by lookup.)  */
! #define LOOKUP_HIDDEN (LOOKUP_CONSTRUCTOR_CALLABLE << 1)
  /* Prefer that the lvalue be treated as an rvalue.  */
  #define LOOKUP_PREFER_RVALUE (LOOKUP_HIDDEN << 1)
  
--- 3699,3708 ----
  #define LOOKUP_PREFER_NAMESPACES (1 << 9)
  /* Accept types or namespaces.  */
  #define LOOKUP_PREFER_BOTH (LOOKUP_PREFER_TYPES | LOOKUP_PREFER_NAMESPACES)
  /* Return friend declarations and un-declared builtin functions.
     (Normally, these entities are registered in the symbol table, but
     not found by lookup.)  */
! #define LOOKUP_HIDDEN (LOOKUP_PREFER_NAMESPACES << 1)
  /* Prefer that the lvalue be treated as an rvalue.  */
  #define LOOKUP_PREFER_RVALUE (LOOKUP_HIDDEN << 1)
  
Index: testsuite/g++.dg/init/copy7.C
===================================================================
*** testsuite/g++.dg/init/copy7.C	(revision 129215)
--- testsuite/g++.dg/init/copy7.C	(working copy)
***************
*** 1,19 ****
- // { dg-options "-std=c++98" }
- // PR c++/12226
- 
- class foo {
- private:
-   foo(const foo &); // { dg-error "" }
- public:
-   foo();
- };
- const foo &bar = foo(); // { dg-error "" }
- 
- class derived : public foo {
- private:
-   derived(const derived&);  // { dg-error "" }
- public:
-   derived();
- };
- 
- const foo& baz = derived(); // { dg-error "" }
--- 0 ----
Index: testsuite/g++.dg/overload/reftemp1.C
===================================================================
*** testsuite/g++.dg/overload/reftemp1.C	(revision 0)
--- testsuite/g++.dg/overload/reftemp1.C	(revision 0)
***************
*** 0 ****
--- 1,13 ----
+ // PR c++/25950
+ 
+ struct X {
+   X();
+   explicit X(const X&);
+ };
+ 
+ void g(const X&);
+ 
+ int main()
+ {
+   g(X());
+ }
Index: testsuite/g++.dg/overload/reftemp2.C
===================================================================
*** testsuite/g++.dg/overload/reftemp2.C	(revision 0)
--- testsuite/g++.dg/overload/reftemp2.C	(revision 0)
***************
*** 0 ****
--- 1,23 ----
+ // DR 391 says that we always bind a reference to the base subobject; it is
+ // incorrect to call the A copy constructor to initialize the parameter of
+ // f.
+ 
+ int fail;
+ 
+ struct A {
+   A() { }
+   A(const A&) { fail = 1; }
+ };
+ struct B : public A { };
+ struct X {
+   operator B() { return B(); }
+ };
+ X x;
+ 
+ void f (const A&) { }
+ 
+ int main()
+ {
+   f(x);
+   return fail;
+ }