[PATCH] c++: shortcut bad convs during overload resolution, part 2 [PR101904]

[PATCH] c++: shortcut bad convs during overload resolution, part 2 [PR101904]

[Patrick Palka]

The r12-3346 patch makes us avoid computing excess argument conversions
during overload resolution, but only when it turns out there's a
strictly viable candidate in the overload set.  If there is no such
candidate then we still need to compute more conversions than strictly
necessary because subsequent conversions after the first bad conversion
can turn a non-strictly viable candidate into unviable one, and that
affects the outcome of overload resolution and the behavior of its
callers (in light of -fpermissive).

But at least in a SFINAE context, the distinction between a non-strictly
viable and an unviable candidate shouldn't matter all that much since
performing a bad conversion is always an error (even with -fpermissive),
and so forming a call to a non-strictly viable candidate will end up
being a SFINAE error anyway, just like in the unviable case.  Hence a
non-strictly viable candidate is effectively unviable (in a SFINAE
context), and we don't really need to distinguish between the two kinds.
We can take advantage of this observation to avoid computing excess
argument conversions even when there's no strictly viable candidate in
the overload set.

This patch implements this idea.  We usually detect a SFINAE context by
looking for the absence of the tf_error flag, but that's not specific
enough: we can also get here from build_user_type_conversion with
tf_error cleared, and there the distinction between a non-strictly
viable candidate and an unviable candidate still matters (it determines
whether a user-defined conversion is bad or just doesn't exist).  So this
patch sets and checks for the tf_conv flag to detect this situation too,
which avoids regressing conv2.C below.

Unlike the previous patch, this one does change the outcome of overload
resolution, but it should do so only in a way that preserves backwards
compatibility with -fpermissive.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
for trunk?

	PR c++/101904

gcc/cp/ChangeLog:

	* call.c (build_user_type_conversion_1): Add tf_conv to complain.
	(add_candidates): When in a SFINAE context, instead of adding a
	candidate to bad_fns just mark it unviable.

gcc/testsuite/ChangeLog:

	* g++.dg/ext/conv2.C: New test.
	* g++.dg/template/conv17.C: Augment test.
---
 gcc/cp/call.c                          | 17 +++++++++++++++--
 gcc/testsuite/g++.dg/ext/conv2.C       | 13 +++++++++++++
 gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
 3 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ext/conv2.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b6011c1a282..ab0d118e34e 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -4175,6 +4175,9 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
   flags |= LOOKUP_NO_CONVERSION;
   if (BRACE_ENCLOSED_INITIALIZER_P (expr))
     flags |= LOOKUP_NO_NARROWING;
+  /* Prevent add_candidates from treating a non-strictly viable candidate
+     as an unviable one.  */
+  complain |= tf_conv;
 
   /* It's OK to bind a temporary for converting constructor arguments, but
      not in converting the return value of a conversion operator.  */
@@ -6232,8 +6235,18 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
 	     stopped at the first bad conversion).  Add the function to BAD_FNS
 	     to fully reconsider later if we don't find any strictly viable
 	     candidates.  */
-	  bad_fns = lookup_add (fn, bad_fns);
-	  *candidates = (*candidates)->next;
+	  if (complain & (tf_error | tf_conv))
+	    {
+	      bad_fns = lookup_add (fn, bad_fns);
+	      *candidates = (*candidates)->next;
+	    }
+	  else
+	    /* But if we're in a SFINAE context, just mark this candidate as
+	       unviable outright and avoid potentially reconsidering it.
+	       This is safe to do since performing a bad conversion is always
+	       erroneous in a SFINAE context (even with -fpermissive), so a
+	       non-strictly viable candidate is effectively unviable anyway.  */
+	    cand->viable = 0;
 	}
     }
   if (which == non_templates && !seen_perfect)
diff --git a/gcc/testsuite/g++.dg/ext/conv2.C b/gcc/testsuite/g++.dg/ext/conv2.C
new file mode 100644
index 00000000000..baf2a43b2ae
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/conv2.C
@@ -0,0 +1,13 @@
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-fpermissive" }
+
+struct A {
+  A(int*, int);
+};
+
+void f(A);
+
+int main() {
+  const int n = 0;
+  f({&n, 42}); // { dg-warning "invalid conversion from 'const int\\*' to 'int\\*'" }
+}
diff --git a/gcc/testsuite/g++.dg/template/conv17.C b/gcc/testsuite/g++.dg/template/conv17.C
index ba012c9d1fa..e40da8f1f24 100644
--- a/gcc/testsuite/g++.dg/template/conv17.C
+++ b/gcc/testsuite/g++.dg/template/conv17.C
@@ -53,4 +53,11 @@ concept D = requires (const T t) {
 };
 
 static_assert(D<C>);
+
+// Test that when there's no strictly viable candidate and we're in a
+// SFINAE context, we still stop at the first bad argument conversion.
+template<class T>
+concept E = requires (T t) { T().h(nullptr); };
+
+static_assert(!E<C>);
 #endif
-- 
2.33.0.328.g8b7c11b866

Re: [PATCH] c++: shortcut bad convs during overload resolution, part 2 [PR101904]

[Jason Merrill]

On 9/12/21 15:45, Patrick Palka wrote:
> The r12-3346 patch makes us avoid computing excess argument conversions
> during overload resolution, but only when it turns out there's a
> strictly viable candidate in the overload set.  If there is no such
> candidate then we still need to compute more conversions than strictly
> necessary because subsequent conversions after the first bad conversion
> can turn a non-strictly viable candidate into unviable one, and that
> affects the outcome of overload resolution and the behavior of its
> callers (in light of -fpermissive).
> 
> But at least in a SFINAE context, the distinction between a non-strictly
> viable and an unviable candidate shouldn't matter all that much since
> performing a bad conversion is always an error (even with -fpermissive),
> and so forming a call to a non-strictly viable candidate will end up
> being a SFINAE error anyway, just like in the unviable case.  Hence a
> non-strictly viable candidate is effectively unviable (in a SFINAE
> context), and we don't really need to distinguish between the two kinds.
> We can take advantage of this observation to avoid computing excess
> argument conversions even when there's no strictly viable candidate in
> the overload set.
> 
> This patch implements this idea.  We usually detect a SFINAE context by
> looking for the absence of the tf_error flag, but that's not specific
> enough: we can also get here from build_user_type_conversion with
> tf_error cleared, and there the distinction between a non-strictly
> viable candidate and an unviable candidate still matters (it determines
> whether a user-defined conversion is bad or just doesn't exist).  So this
> patch sets and checks for the tf_conv flag to detect this situation too,
> which avoids regressing conv2.C below.
> 
> Unlike the previous patch, this one does change the outcome of overload
> resolution, but it should do so only in a way that preserves backwards
> compatibility with -fpermissive.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
> for trunk?

OK.

> 	PR c++/101904
> 
> gcc/cp/ChangeLog:
> 
> 	* call.c (build_user_type_conversion_1): Add tf_conv to complain.
> 	(add_candidates): When in a SFINAE context, instead of adding a
> 	candidate to bad_fns just mark it unviable.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/ext/conv2.C: New test.
> 	* g++.dg/template/conv17.C: Augment test.
> ---
>   gcc/cp/call.c                          | 17 +++++++++++++++--
>   gcc/testsuite/g++.dg/ext/conv2.C       | 13 +++++++++++++
>   gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
>   3 files changed, 35 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/ext/conv2.C
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index b6011c1a282..ab0d118e34e 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -4175,6 +4175,9 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
>     flags |= LOOKUP_NO_CONVERSION;
>     if (BRACE_ENCLOSED_INITIALIZER_P (expr))
>       flags |= LOOKUP_NO_NARROWING;
> +  /* Prevent add_candidates from treating a non-strictly viable candidate
> +     as an unviable one.  */
> +  complain |= tf_conv;
>   
>     /* It's OK to bind a temporary for converting constructor arguments, but
>        not in converting the return value of a conversion operator.  */
> @@ -6232,8 +6235,18 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
>   	     stopped at the first bad conversion).  Add the function to BAD_FNS
>   	     to fully reconsider later if we don't find any strictly viable
>   	     candidates.  */
> -	  bad_fns = lookup_add (fn, bad_fns);
> -	  *candidates = (*candidates)->next;
> +	  if (complain & (tf_error | tf_conv))
> +	    {
> +	      bad_fns = lookup_add (fn, bad_fns);
> +	      *candidates = (*candidates)->next;
> +	    }
> +	  else
> +	    /* But if we're in a SFINAE context, just mark this candidate as
> +	       unviable outright and avoid potentially reconsidering it.
> +	       This is safe to do since performing a bad conversion is always
> +	       erroneous in a SFINAE context (even with -fpermissive), so a
> +	       non-strictly viable candidate is effectively unviable anyway.  */
> +	    cand->viable = 0;
>   	}
>       }
>     if (which == non_templates && !seen_perfect)
> diff --git a/gcc/testsuite/g++.dg/ext/conv2.C b/gcc/testsuite/g++.dg/ext/conv2.C
> new file mode 100644
> index 00000000000..baf2a43b2ae
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ext/conv2.C
> @@ -0,0 +1,13 @@
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options "-fpermissive" }
> +
> +struct A {
> +  A(int*, int);
> +};
> +
> +void f(A);
> +
> +int main() {
> +  const int n = 0;
> +  f({&n, 42}); // { dg-warning "invalid conversion from 'const int\\*' to 'int\\*'" }
> +}
> diff --git a/gcc/testsuite/g++.dg/template/conv17.C b/gcc/testsuite/g++.dg/template/conv17.C
> index ba012c9d1fa..e40da8f1f24 100644
> --- a/gcc/testsuite/g++.dg/template/conv17.C
> +++ b/gcc/testsuite/g++.dg/template/conv17.C
> @@ -53,4 +53,11 @@ concept D = requires (const T t) {
>   };
>   
>   static_assert(D<C>);
> +
> +// Test that when there's no strictly viable candidate and we're in a
> +// SFINAE context, we still stop at the first bad argument conversion.
> +template<class T>
> +concept E = requires (T t) { T().h(nullptr); };
> +
> +static_assert(!E<C>);
>   #endif
>