Re: [PATCH v2] c++: Make convert_like complain about bad ck_ref_bind again [PR95789]

[Marek Polacek <polacek@redhat.com>]

On Fri, Jul 03, 2020 at 05:24:34PM -0400, Jason Merrill via Gcc-patches wrote:
> On 6/22/20 10:09 PM, Marek Polacek wrote:
> > convert_like issues errors about bad_p conversions at the beginning
> > of the function, but in the ck_ref_bind case, it only issues them
> > after we've called convert_like on the next conversion.
> > 
> > This doesn't work as expected since r10-7096 because when we see
> > a conversion from/to class type in a template, we return early, thereby
> > missing the error, and a bad_p conversion goes by undetected.  That
> > made the attached test to compile even though it should not.
> 
> Hmm, why isn't there an error at instantiation time?

We threw away the result: we're called from

12213       if (complain & tf_error)
12214         {
12215           if (conv)
12216             convert_like (conv, expr, complain);
...
12228       return error_mark_node;

and convert_like never saw converting this->f to B& again when instantiating.

> Though giving an error at template parsing time is definitely preferable.

Yup.

> > I had thought that I could just move the ck_ref_bind/bad_p errors
> > above to the rest of them, but that regressed diagnostics because
> > expr then wasn't converted yet by the nested convert_like_real call.
> 
> Yeah, the early section is really just for scalar conversions.
> 
> It would probably be good to do normal processing for all other bad
> conversions and only afterward build the IMPLICIT_CONV_EXPR if we aren't
> returning error_mark_node.

Ok, so that if we add more bad_p errors, we won't run into this again.

Unfortunately it's a bit ugly.  I could introduce a RETURN macro to
use RETURN (expr); instead of what I have now, but it wouldn't be simply
"conv_expr ? conv_expr : expr", because if we have error_mark_node, we
want to return that, not conv_expr.  Does that seem worth it?
(I wish I could at least use the op0 ?: op1 GNU extension.)

I've also added another test.

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

-- >8 --
convert_like issues errors about bad_p conversions at the beginning
of the function, but in the ck_ref_bind case, it only issues them
after we've called convert_like on the next conversion.

This doesn't work as expected since r10-7096 because when we see
a conversion from/to class type in a template, we return early, thereby
missing the error, and a bad_p conversion goes by undetected.  That
made the attached test to compile even though it should not.

I had thought that I could just move the ck_ref_bind/bad_p errors
above to the rest of them, but that regressed diagnostics because
expr then wasn't converted yet by the nested convert_like_real call.

So, for bad_p conversions, do the normal processing, but still return
the IMPLICIT_CONV_EXPR to avoid introducing trees that the template
processing can't handle well.

gcc/cp/ChangeLog:

	PR c++/95789
	* call.c (convert_like_real): Do the normal processing for
	conversion that are bad_p.  Return the IMPLICIT_CONV_EXPR
	instead of EXPR if we're processing a bad_p conversion in
	a template.

gcc/testsuite/ChangeLog:

	PR c++/95789
	* g++.dg/conversion/ref4.C: New test.
	* g++.dg/conversion/ref5.C: New test.
---
 gcc/cp/call.c                          | 47 +++++++++++++++-----------
 gcc/testsuite/g++.dg/conversion/ref4.C | 22 ++++++++++++
 gcc/testsuite/g++.dg/conversion/ref5.C | 14 ++++++++
 3 files changed, 64 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref4.C
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref5.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 5341a572980..65565fc90a8 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7400,12 +7400,19 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
      so use an IMPLICIT_CONV_EXPR for this conversion.  We would have
      created such codes e.g. when calling a user-defined conversion
      function.  */
+  tree conv_expr = NULL_TREE;
   if (processing_template_decl
       && convs->kind != ck_identity
       && (CLASS_TYPE_P (totype) || CLASS_TYPE_P (TREE_TYPE (expr))))
     {
-      expr = build1 (IMPLICIT_CONV_EXPR, totype, expr);
-      return convs->kind == ck_ref_bind ? expr : convert_from_reference (expr);
+      conv_expr = build1 (IMPLICIT_CONV_EXPR, totype, expr);
+      if (convs->kind != ck_ref_bind)
+	conv_expr = convert_from_reference (conv_expr);
+      if (!convs->bad_p)
+	return conv_expr;
+      /* Do the normal processing to give the bad_p errors.  But we still
+	 need to return the IMPLICIT_CONV_EXPR, unless we're returning
+	 error_mark_node.  */
     }
 
   switch (convs->kind)
@@ -7465,7 +7472,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 		TARGET_EXPR_LIST_INIT_P (expr) = true;
 		TARGET_EXPR_DIRECT_INIT_P (expr) = direct;
 	      }
-	    return expr;
+	    return conv_expr ? conv_expr : expr;
 	  }
 
 	/* We don't know here whether EXPR is being used as an lvalue or
@@ -7488,7 +7495,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	      TARGET_EXPR_LIST_INIT_P (expr) = true;
 	  }
 
-	return expr;
+	return conv_expr ? conv_expr : expr;
       }
     case ck_identity:
       if (BRACE_ENCLOSED_INITIALIZER_P (expr))
@@ -7513,7 +7520,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	   continue to warn about uses of EXPR as an integer, rather than as a
 	   pointer.  */
 	expr = build_int_cst (totype, 0);
-      return expr;
+      return conv_expr ? conv_expr : expr;
     case ck_ambig:
       /* We leave bad_p off ck_ambig because overload resolution considers
 	 it valid, it just fails when we try to perform it.  So we need to
@@ -7585,7 +7592,8 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	field = next_initializable_field (DECL_CHAIN (field));
 	CONSTRUCTOR_APPEND_ELT (vec, field, size_int (len));
 	tree new_ctor = build_constructor (totype, vec);
-	return get_target_expr_sfinae (new_ctor, complain);
+	return (conv_expr ? conv_expr
+			  : get_target_expr_sfinae (new_ctor, complain));
       }
 
     case ck_aggr:
@@ -7598,14 +7606,14 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	  imag = perform_implicit_conversion (TREE_TYPE (totype),
 					      imag, complain);
 	  expr = build2 (COMPLEX_EXPR, totype, real, imag);
-	  return expr;
+	  return conv_expr ? conv_expr : expr;
 	}
       expr = reshape_init (totype, expr, complain);
       expr = get_target_expr_sfinae (digest_init (totype, expr, complain),
 				     complain);
       if (expr != error_mark_node)
 	TARGET_EXPR_LIST_INIT_P (expr) = true;
-      return expr;
+      return conv_expr ? conv_expr : expr;
 
     default:
       break;
@@ -7634,19 +7642,19 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	}
 
       if (! MAYBE_CLASS_TYPE_P (totype))
-	return expr;
+	return conv_expr ? conv_expr : expr;
 
       /* Don't introduce copies when passing arguments along to the inherited
 	 constructor.  */
       if (current_function_decl
 	  && flag_new_inheriting_ctors
 	  && DECL_INHERITED_CTOR (current_function_decl))
-	return expr;
+	return conv_expr ? conv_expr : expr;
 
       if (TREE_CODE (expr) == TARGET_EXPR
 	  && TARGET_EXPR_LIST_INIT_P (expr))
 	/* Copy-list-initialization doesn't actually involve a copy.  */
-	return expr;
+	return conv_expr ? conv_expr : expr;
 
       /* Fall through.  */
     case ck_base:
@@ -7657,7 +7665,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	  /* Build an expression for `*((base*) &expr)'.  */
 	  expr = convert_to_base (expr, totype,
 				  !c_cast_p, /*nonnull=*/true, complain);
-	  return expr;
+	  return conv_expr ? conv_expr : expr;
 	}
 
       /* Copy-initialization where the cv-unqualified version of the source
@@ -7686,7 +7694,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	  maybe_inform_about_fndecl_for_bogus_argument_init (fn, argnum);
 	}
 
-      return build_cplus_new (totype, expr, complain);
+      return conv_expr ? conv_expr : build_cplus_new (totype, expr, complain);
 
     case ck_ref_bind:
       {
@@ -7821,16 +7829,16 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	expr = cp_convert (build_pointer_type (TREE_TYPE (ref_type)),
 			   expr, complain);
 	/* Convert the pointer to the desired reference type.  */
-	return build_nop (ref_type, expr);
+	return conv_expr ? conv_expr : build_nop (ref_type, expr);
       }
 
     case ck_lvalue:
-      return decay_conversion (expr, complain);
+      return conv_expr ? conv_expr : decay_conversion (expr, complain);
 
     case ck_fnptr:
       /* ??? Should the address of a transaction-safe pointer point to the TM
         clone, and this conversion look up the primary function?  */
-      return build_nop (totype, expr);
+      return conv_expr ? conv_expr : build_nop (totype, expr);
 
     case ck_qual:
       /* Warn about deprecated conversion if appropriate.  */
@@ -7845,11 +7853,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
       if (convs->base_p)
 	expr = convert_to_base (expr, totype, !c_cast_p,
 				/*nonnull=*/false, complain);
-      return build_nop (totype, expr);
+      return conv_expr ? conv_expr : build_nop (totype, expr);
 
     case ck_pmem:
-      return convert_ptrmem (totype, expr, /*allow_inverse_p=*/false,
+      expr = convert_ptrmem (totype, expr, /*allow_inverse_p=*/false,
 			     c_cast_p, complain);
+      return conv_expr ? conv_expr : expr;
 
     default:
       break;
@@ -7866,7 +7875,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
   else
     expr = cp_convert (totype, expr, complain);
 
-  return expr;
+  return conv_expr ? conv_expr : expr;
 }
 
 /* ARG is being passed to a varargs function.  Perform any conversions
diff --git a/gcc/testsuite/g++.dg/conversion/ref4.C b/gcc/testsuite/g++.dg/conversion/ref4.C
new file mode 100644
index 00000000000..464a4cf6c0f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref4.C
@@ -0,0 +1,22 @@
+// PR c++/95789
+// { dg-do compile { target c++11 } }
+
+struct B {
+    int n;
+};
+
+template <typename T>
+struct A {
+    B& get() const { return f; } // { dg-error "binding reference" }
+
+    B f;
+};
+
+int main() {
+    A<int> a;
+    a.f = {};
+
+    a.get().n = 10;
+    if (a.f.n != 0)
+      __builtin_abort();
+}
diff --git a/gcc/testsuite/g++.dg/conversion/ref5.C b/gcc/testsuite/g++.dg/conversion/ref5.C
new file mode 100644
index 00000000000..0042acd0670
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref5.C
@@ -0,0 +1,14 @@
+// PR c++/96104
+
+template <typename T> void fn(T &);
+class E {};
+struct F {
+  template <typename T> void mfn(T t) { t, fn(E()); } // { dg-error "cannot bind non-const lvalue reference" }
+};
+int
+main()
+{
+  E e;
+  F f;
+  f.mfn(e);
+}

base-commit: 760df6d296b8fc59796f42dca5eb14012fbfa28b
-- 
2.26.2