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

[Marek Polacek <polacek@redhat.com>]

On Sat, Jul 11, 2020 at 10:32:55AM -0400, Jason Merrill via Gcc-patches wrote:
> On 7/8/20 4:35 PM, Marek Polacek wrote:
> > 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.)
> 
> Or introduce a wrapper function that handles IMPLICIT_CONV_EXPR?

Perhaps like this?  convert_like_real_1 is unsightly and I have 
plans to fix that (and get rid of the convert_like macro), but
that's a clean-up, not a bug fix so it's not included in this patch.

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

-- >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.  This I achieved by adding a wrapper
function.

gcc/cp/ChangeLog:

	PR c++/95789
	PR c++/96104
	PR c++/96179
	* call.c (convert_like_real_1): Renamed from convert_like_real.
	(convert_like_real): New wrapper for convert_like_real_1.

gcc/testsuite/ChangeLog:

	PR c++/95789
	PR c++/96104
	PR c++/96179
	* g++.dg/conversion/ref4.C: New test.
	* g++.dg/conversion/ref5.C: New test.
	* g++.dg/conversion/ref6.C: New test.
---
 gcc/cp/call.c                          | 54 ++++++++++++++++++--------
 gcc/testsuite/g++.dg/conversion/ref4.C | 22 +++++++++++
 gcc/testsuite/g++.dg/conversion/ref5.C | 14 +++++++
 gcc/testsuite/g++.dg/conversion/ref6.C | 24 ++++++++++++
 4 files changed, 98 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref4.C
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref5.C
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref6.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 5341a572980..6d5d5e801a5 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -171,6 +171,8 @@ static tree build_over_call (struct z_candidate *, int, tsubst_flags_t);
 		     /*c_cast_p=*/false, (COMPLAIN))
 static tree convert_like_real (conversion *, tree, tree, int, bool,
 			       bool, tsubst_flags_t);
+static tree convert_like_real_1 (conversion *, tree, tree, int, bool,
+				 bool, tsubst_flags_t);
 static void op_error (const op_location_t &, enum tree_code, enum tree_code,
 		      tree, tree, tree, bool);
 static struct z_candidate *build_user_type_conversion_1 (tree, tree, int,
@@ -7281,6 +7283,39 @@ maybe_warn_array_conv (location_t loc, conversion *c, tree expr)
 	     "are only available with %<-std=c++20%> or %<-std=gnu++20%>");
 }
 
+/* Wrapper for convert_like_real_1 that handles creating IMPLICIT_CONV_EXPR.  */
+
+static tree
+convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
+		   bool issue_conversion_warnings,
+		   bool c_cast_p, tsubst_flags_t complain)
+{
+  /* Creating &TARGET_EXPR<> in a template breaks when substituting,
+     and creating a CALL_EXPR in a template breaks in finish_call_expr
+     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 (convs->type) || CLASS_TYPE_P (TREE_TYPE (expr))))
+    {
+      conv_expr = build1 (IMPLICIT_CONV_EXPR, convs->type, 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.  */
+    }
+  expr = convert_like_real_1 (convs, expr, fn, argnum,
+			      issue_conversion_warnings, c_cast_p, complain);
+  if (expr == error_mark_node)
+    return error_mark_node;
+  return conv_expr ? conv_expr : expr;
+}
+
 /* Perform the conversions in CONVS on the expression EXPR.  FN and
    ARGNUM are used for diagnostics.  ARGNUM is zero based, -1
    indicates the `this' argument of a method.  INNER is nonzero when
@@ -7292,9 +7327,9 @@ maybe_warn_array_conv (location_t loc, conversion *c, tree expr)
    conversions to inaccessible bases are permitted.  */
 
 static tree
-convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
-		   bool issue_conversion_warnings,
-		   bool c_cast_p, tsubst_flags_t complain)
+convert_like_real_1 (conversion *convs, tree expr, tree fn, int argnum,
+		     bool issue_conversion_warnings,
+		     bool c_cast_p, tsubst_flags_t complain)
 {
   tree totype = convs->type;
   diagnostic_t diag_kind;
@@ -7395,19 +7430,6 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
   if (issue_conversion_warnings && (complain & tf_warning))
     conversion_null_warnings (totype, expr, fn, argnum);
 
-  /* Creating &TARGET_EXPR<> in a template breaks when substituting,
-     and creating a CALL_EXPR in a template breaks in finish_call_expr
-     so use an IMPLICIT_CONV_EXPR for this conversion.  We would have
-     created such codes e.g. when calling a user-defined conversion
-     function.  */
-  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);
-    }
-
   switch (convs->kind)
     {
     case ck_user:
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);
+}
diff --git a/gcc/testsuite/g++.dg/conversion/ref6.C b/gcc/testsuite/g++.dg/conversion/ref6.C
new file mode 100644
index 00000000000..fc87199053c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref6.C
@@ -0,0 +1,24 @@
+// PR c++/96179
+// { dg-do compile { target c++11 } }
+
+template<typename T> struct vector
+{
+  void push_back(T) { }
+};
+
+struct dummy{
+        int a;
+};
+
+void Modify_Dummy(dummy &d){
+        d.a=1;
+}
+
+template <bool bla=true> void Templated_Function(){
+        vector<dummy> A;
+        A.push_back(Modify_Dummy(dummy{0})); // { dg-error "cannot bind non-const lvalue reference" }
+}
+
+int main(){
+        Templated_Function();
+}

base-commit: 9cba898481368ce16c6a2d30ef781a82dce27c55
-- 
2.26.2