[PATCH] c++: Check constraints before instantiation from mark_used [PR95132]

[PATCH] c++: Check constraints before instantiation from mark_used [PR95132]

[Patrick Palka]

This makes mark_used check constraints of a function _before_ calling
maybe_instantiate_decl, so that we don't try instantiating a function
(as part of return type deduction) with unsatisfied constraints.

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

gcc/cp/ChangeLog:

	PR c++/95132
	* decl2.c (mark_used): Move up the constraints_satisfied_p check
	so that it happens before calling maybe_instantiate_decl.

gcc/testsuite/ChangeLog:

	PR c++/95132
	* g++.dg/cpp2a/concepts-fn7.C: New test.
---
 gcc/cp/decl2.c                            | 30 +++++++++++------------
 gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C | 11 +++++++++
 2 files changed, 26 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C

diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index 2f0d6370146..de2956aa5f0 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -5604,6 +5604,21 @@ mark_used (tree decl, tsubst_flags_t complain)
   if (DECL_ODR_USED (decl))
     return true;
 
+  if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL
+      && !constraints_satisfied_p (decl))
+    {
+      if (complain & tf_error)
+	{
+	  auto_diagnostic_group d;
+	  error ("use of function %qD with unsatisfied constraints",
+		 decl);
+	  location_t loc = DECL_SOURCE_LOCATION (decl);
+	  inform (loc, "declared here");
+	  diagnose_constraints (loc, decl, NULL_TREE);
+	}
+      return false;
+    }
+
   /* Normally, we can wait until instantiation-time to synthesize DECL.
      However, if DECL is a static data member initialized with a constant
      or a constexpr function, we need it right now because a reference to
@@ -5614,21 +5629,6 @@ mark_used (tree decl, tsubst_flags_t complain)
      directly.  */
   maybe_instantiate_decl (decl);
 
-  if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL
-      && !constraints_satisfied_p (decl))
-    {
-      if (complain & tf_error)
-	{
-	  auto_diagnostic_group d;
-	  error ("use of function %qD with unsatisfied constraints",
-		 decl);
-	  location_t loc = DECL_SOURCE_LOCATION (decl);
-	  inform (loc, "declared here");
-	  diagnose_constraints (loc, decl, NULL_TREE);
-	}
-      return false;
-    }
-
   if (processing_template_decl || in_template_function ())
     return true;
 
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C
new file mode 100644
index 00000000000..7fad6f374b7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C
@@ -0,0 +1,11 @@
+// PR c++/95132
+// { dg-do compile { target c++20 } }
+
+template<typename T> struct A {
+  static auto f() requires false { return T::fail; }
+};
+
+template<typename T>
+constexpr bool v = requires { A<T>::f(); };
+
+static_assert(!v<int>);
-- 
2.29.0.rc0

[PATCH] c++: Check constraints only on candidate conversion functions

[Patrick Palka]

In the testcase below, we're overeagerly checking the constraints on
the conversion function B<int>::operator bool() as part of finding an
implicit conversion sequence from B<int> to const A&.

This behavior seems to be nonconforming because according to
[over.match.copy] and [over.match.conv], only those conversion functions
which yield a type that can be converted to the target type via a
standard conversion sequence are candidate functions, and according to
[over.match.viable], only candidate functions get their constraints
checked.

This patch fixes this by swapping the order of the calls to
add_candidates and implicit_conversion in build_user_type_conversion_1
when the conversion function has a non-dependent result type.
(Conversion function templates with a dependent result type already get
handled appropriately in add_template_candidate, it seems.)

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

gcc/cp/ChangeLog:

	* call.c (build_user_type_conversion_1): When the conversion
	function has a non-dependent result type, pre-check the
	conversion sequence that would follow the conversion function.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-conv3.C: New test.
---
 gcc/cp/call.c                               | 33 +++++++++++++++++----
 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C | 14 +++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index bd662518958..6d955ea083f 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -4099,6 +4099,23 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
       if (TYPE_REF_P (totype))
 	convflags |= LOOKUP_NO_TEMP_BIND;
 
+      conversion *pre_ics = NULL;
+      if (!uses_template_parms (TREE_TYPE (conv_fns)))
+	{
+	  /* A conversion function is a candidate function only if it
+	     yields a type that can be converted to the target type.
+	     We check this for conversion functions with non-dependent
+	     result type now to disregard the non-candidate functions
+	     before calling add_candidates.  */
+	  pre_ics = implicit_conversion (totype,
+					 TREE_TYPE (conv_fns),
+					 0,
+					 /*c_cast_p=*/false, convflags,
+					 complain);
+	  if (!pre_ics)
+	    continue;
+	}
+
       old_candidates = candidates;
       add_candidates (TREE_VALUE (conv_fns), first_arg, NULL, totype,
 		      NULL_TREE, false,
@@ -4112,12 +4129,16 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
 	    continue;
 
 	  tree rettype = TREE_TYPE (TREE_TYPE (cand->fn));
-	  conversion *ics
-	    = implicit_conversion (totype,
-				   rettype,
-				   0,
-				   /*c_cast_p=*/false, convflags,
-				   complain);
+	  conversion *ics;
+	  if (pre_ics)
+	    /* We've already computed the conversion sequence above.  */
+	    ics = pre_ics;
+	  else
+	    ics = implicit_conversion (totype,
+				       rettype,
+				       0,
+				       /*c_cast_p=*/false, convflags,
+				       complain);
 
 	  /* If LOOKUP_NO_TEMP_BIND isn't set, then this is
 	     copy-initialization.  In that case, "The result of the
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
new file mode 100644
index 00000000000..bcbc08fe869
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
@@ -0,0 +1,14 @@
+// { dg-do compile { target c++20 } }
+
+template <class T>
+struct Error { static constexpr auto value = T::value; };
+
+struct A { A(const A&); };
+
+template <class T>
+struct B { operator bool() requires Error<T>::value; };
+
+template <class T>
+concept C = requires (B<T> b) { A(b); };
+
+static_assert(!C<int>);
-- 
2.29.0.rc0

Re: [PATCH] c++: Check constraints only on candidate conversion functions

[Jason Merrill]

On 10/26/20 5:37 PM, Patrick Palka wrote:
> In the testcase below, we're overeagerly checking the constraints on
> the conversion function B<int>::operator bool() as part of finding an
> implicit conversion sequence from B<int> to const A&.
> 
> This behavior seems to be nonconforming because according to
> [over.match.copy] and [over.match.conv], only those conversion functions
> which yield a type that can be converted to the target type via a
> standard conversion sequence are candidate functions, and according to
> [over.match.viable], only candidate functions get their constraints
> checked.

Hmm.  I agree with your reading of the standard, but I'm not sure this 
is what we actually want.  It's the reverse of the agreed direction for 
CWG2369, to check constraints before non-dependent conversions for 
function arguments, and goes against the more general principle that the 
fix for unwanted instantiation should be additional constraints.

A different testcase would succeed before your patch, and fail after:

template <class T>
struct Error { static constexpr auto value = T::value; };

struct A { A(const A&); };

template <class T>
struct B { operator Error<T>*() requires false; };

template <class T>
concept C = requires (B<T> b) { (A*)b; };

static_assert(!C<int>);

I've raised this on the reflector.

> This patch fixes this by swapping the order of the calls to
> add_candidates and implicit_conversion in build_user_type_conversion_1
> when the conversion function has a non-dependent result type.
> (Conversion function templates with a dependent result type already get
> handled appropriately in add_template_candidate, it seems.)

Testcase?

But yes, for dependent result types if there isn't a conversion, 
deduction will fail.

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

> gcc/cp/ChangeLog:
> 
> 	* call.c (build_user_type_conversion_1): When the conversion
> 	function has a non-dependent result type, pre-check the
> 	conversion sequence that would follow the conversion function.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-conv3.C: New test.
> ---
>   gcc/cp/call.c                               | 33 +++++++++++++++++----
>   gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C | 14 +++++++++
>   2 files changed, 41 insertions(+), 6 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index bd662518958..6d955ea083f 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -4099,6 +4099,23 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
>         if (TYPE_REF_P (totype))
>   	convflags |= LOOKUP_NO_TEMP_BIND;
>   
> +      conversion *pre_ics = NULL;
> +      if (!uses_template_parms (TREE_TYPE (conv_fns)))
> +	{
> +	  /* A conversion function is a candidate function only if it
> +	     yields a type that can be converted to the target type.
> +	     We check this for conversion functions with non-dependent
> +	     result type now to disregard the non-candidate functions
> +	     before calling add_candidates.  */
> +	  pre_ics = implicit_conversion (totype,
> +					 TREE_TYPE (conv_fns),
> +					 0,
> +					 /*c_cast_p=*/false, convflags,
> +					 complain);
> +	  if (!pre_ics)
> +	    continue;
> +	}
> +
>         old_candidates = candidates;
>         add_candidates (TREE_VALUE (conv_fns), first_arg, NULL, totype,
>   		      NULL_TREE, false,
> @@ -4112,12 +4129,16 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
>   	    continue;
>   
>   	  tree rettype = TREE_TYPE (TREE_TYPE (cand->fn));
> -	  conversion *ics
> -	    = implicit_conversion (totype,
> -				   rettype,
> -				   0,
> -				   /*c_cast_p=*/false, convflags,
> -				   complain);
> +	  conversion *ics;
> +	  if (pre_ics)
> +	    /* We've already computed the conversion sequence above.  */
> +	    ics = pre_ics;
> +	  else
> +	    ics = implicit_conversion (totype,
> +				       rettype,
> +				       0,
> +				       /*c_cast_p=*/false, convflags,
> +				       complain);
>   
>   	  /* If LOOKUP_NO_TEMP_BIND isn't set, then this is
>   	     copy-initialization.  In that case, "The result of the
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
> new file mode 100644
> index 00000000000..bcbc08fe869
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
> @@ -0,0 +1,14 @@
> +// { dg-do compile { target c++20 } }
> +
> +template <class T>
> +struct Error { static constexpr auto value = T::value; };
> +
> +struct A { A(const A&); };
> +
> +template <class T>
> +struct B { operator bool() requires Error<T>::value; };
> +
> +template <class T>
> +concept C = requires (B<T> b) { A(b); };
> +
> +static_assert(!C<int>);
> 

Re: [PATCH] c++: Check constraints only on candidate conversion functions

[Patrick Palka]

On Tue, 27 Oct 2020, Jason Merrill wrote:

> On 10/26/20 5:37 PM, Patrick Palka wrote:
> > In the testcase below, we're overeagerly checking the constraints on
> > the conversion function B<int>::operator bool() as part of finding an
> > implicit conversion sequence from B<int> to const A&.
> > 
> > This behavior seems to be nonconforming because according to
> > [over.match.copy] and [over.match.conv], only those conversion functions
> > which yield a type that can be converted to the target type via a
> > standard conversion sequence are candidate functions, and according to
> > [over.match.viable], only candidate functions get their constraints
> > checked.
> 
> Hmm.  I agree with your reading of the standard, but I'm not sure this is what
> we actually want.  It's the reverse of the agreed direction for CWG2369, to
> check constraints before non-dependent conversions for function arguments, and
> goes against the more general principle that the fix for unwanted
> instantiation should be additional constraints.
> 
> A different testcase would succeed before your patch, and fail after:
> 
> template <class T>
> struct Error { static constexpr auto value = T::value; };
> 
> struct A { A(const A&); };
> 
> template <class T>
> struct B { operator Error<T>*() requires false; };
> 
> template <class T>
> concept C = requires (B<T> b) { (A*)b; };
> 
> static_assert(!C<int>);
> 
> I've raised this on the reflector.

I see, thanks.  I should note I originally hit this issue in some of the
<ranges> testcases while working on
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557237.html
to make us cache satisfaction results aggressively.

Though after some experimentation, I managed to come up with a <ranges>
testcase that misbehaves in the same way, even with GCC 10's
conservative satisfaction caching.  I filed PR libstdc++/96700 and
posted a more detailed analysis of the problem.

> 
> > This patch fixes this by swapping the order of the calls to
> > add_candidates and implicit_conversion in build_user_type_conversion_1
> > when the conversion function has a non-dependent result type.
> > (Conversion function templates with a dependent result type already get
> > handled appropriately in add_template_candidate, it seems.)
> 
> Testcase?
> 
> But yes, for dependent result types if there isn't a conversion, deduction
> will fail.

Ah, indeed.  Here's an updated patch that adds to the testcase a
conversion function template for which deduction fails, to verify we
don't check its constraints either:

-- >8 --

gcc/cp/ChangeLog:

	* call.c (build_user_type_conversion_1): When the conversion
	function has a non-dependent result type, pre-check the
	conversion sequence that would follow the conversion function.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-conv3.C: New test.
---
 gcc/cp/call.c                               | 31 +++++++++++++++++----
 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C | 19 +++++++++++++
 2 files changed, 44 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index bd662518958..01adc5dc2ab 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -4099,6 +4099,22 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
       if (TYPE_REF_P (totype))
 	convflags |= LOOKUP_NO_TEMP_BIND;
 
+      conversion *pre_ics = NULL;
+      if (!uses_template_parms (TREE_TYPE (conv_fns)))
+	{
+	  /* A conversion function is a candidate function only if it
+	     yields a type that can be converted to the target type.
+	     For conversion functions with non-dependent result type,
+	     we check this now to discard non-candidates before calling
+	     add_candidates.  */
+	  pre_ics = implicit_conversion (totype, TREE_TYPE (conv_fns),
+					 /*expr=*/NULL_TREE,
+					 /*c_cast_p=*/false, convflags,
+					 complain);
+	  if (!pre_ics)
+	    continue;
+	}
+
       old_candidates = candidates;
       add_candidates (TREE_VALUE (conv_fns), first_arg, NULL, totype,
 		      NULL_TREE, false,
@@ -4112,12 +4128,15 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
 	    continue;
 
 	  tree rettype = TREE_TYPE (TREE_TYPE (cand->fn));
-	  conversion *ics
-	    = implicit_conversion (totype,
-				   rettype,
-				   0,
-				   /*c_cast_p=*/false, convflags,
-				   complain);
+	  conversion *ics;
+	  if (pre_ics)
+	    /* No need to recompute it.  */
+	    ics = pre_ics;
+	  else
+	    ics = implicit_conversion (totype, rettype,
+				      /*expr=*/NULL_TREE,
+				       /*c_cast_p=*/false, convflags,
+				       complain);
 
 	  /* If LOOKUP_NO_TEMP_BIND isn't set, then this is
 	     copy-initialization.  In that case, "The result of the
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
new file mode 100644
index 00000000000..e0e49c737a8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C
@@ -0,0 +1,19 @@
+// { dg-do compile { target c++20 } }
+
+template <class T>
+struct Error { static constexpr auto value = T::value; };
+
+struct A { A(const A&); };
+
+template <class T>
+struct B {
+  operator bool() requires Error<T>::value;
+
+  template <class U>
+  operator U*() requires Error<T>::value;
+};
+
+template <class T>
+concept C = requires (B<T> b) { A(b); };
+
+static_assert(!C<int>);
-- 
2.29.0.rc0

Re: [PATCH] c++: Check constraints before instantiation from mark_used [PR95132]

[Jason Merrill]

On 10/26/20 5:37 PM, Patrick Palka wrote:
> This makes mark_used check constraints of a function _before_ calling
> maybe_instantiate_decl, so that we don't try instantiating a function
> (as part of return type deduction) with unsatisfied constraints.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk and perhaps the 10 branch?

OK for both.

> gcc/cp/ChangeLog:
> 
> 	PR c++/95132
> 	* decl2.c (mark_used): Move up the constraints_satisfied_p check
> 	so that it happens before calling maybe_instantiate_decl.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/95132
> 	* g++.dg/cpp2a/concepts-fn7.C: New test.
> ---
>   gcc/cp/decl2.c                            | 30 +++++++++++------------
>   gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C | 11 +++++++++
>   2 files changed, 26 insertions(+), 15 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C
> 
> diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
> index 2f0d6370146..de2956aa5f0 100644
> --- a/gcc/cp/decl2.c
> +++ b/gcc/cp/decl2.c
> @@ -5604,6 +5604,21 @@ mark_used (tree decl, tsubst_flags_t complain)
>     if (DECL_ODR_USED (decl))
>       return true;
>   
> +  if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL
> +      && !constraints_satisfied_p (decl))
> +    {
> +      if (complain & tf_error)
> +	{
> +	  auto_diagnostic_group d;
> +	  error ("use of function %qD with unsatisfied constraints",
> +		 decl);
> +	  location_t loc = DECL_SOURCE_LOCATION (decl);
> +	  inform (loc, "declared here");
> +	  diagnose_constraints (loc, decl, NULL_TREE);
> +	}
> +      return false;
> +    }
> +
>     /* Normally, we can wait until instantiation-time to synthesize DECL.
>        However, if DECL is a static data member initialized with a constant
>        or a constexpr function, we need it right now because a reference to
> @@ -5614,21 +5629,6 @@ mark_used (tree decl, tsubst_flags_t complain)
>        directly.  */
>     maybe_instantiate_decl (decl);
>   
> -  if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL
> -      && !constraints_satisfied_p (decl))
> -    {
> -      if (complain & tf_error)
> -	{
> -	  auto_diagnostic_group d;
> -	  error ("use of function %qD with unsatisfied constraints",
> -		 decl);
> -	  location_t loc = DECL_SOURCE_LOCATION (decl);
> -	  inform (loc, "declared here");
> -	  diagnose_constraints (loc, decl, NULL_TREE);
> -	}
> -      return false;
> -    }
> -
>     if (processing_template_decl || in_template_function ())
>       return true;
>   
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C
> new file mode 100644
> index 00000000000..7fad6f374b7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C
> @@ -0,0 +1,11 @@
> +// PR c++/95132
> +// { dg-do compile { target c++20 } }
> +
> +template<typename T> struct A {
> +  static auto f() requires false { return T::fail; }
> +};
> +
> +template<typename T>
> +constexpr bool v = requires { A<T>::f(); };
> +
> +static_assert(!v<int>);
>