[PATCH] c++: ICE with diagnosed constraint recursion [PR100288]

[PATCH] c++: ICE with diagnosed constraint recursion [PR100288]

[Patrick Palka]

When satisfaction_cache::get detects constraint recursion, it asserts
that entry->result is empty.  This makes sense when we're initially
detecting/diagnosing recursion from the inner recursive call, but
aftewards from the outer recursive call the recursion error is treated
like any other SFINAE error encountered during satisfaction, and we set
entry->result to whatever the satisfaction value ended up being.

Perhaps we should keep entry->result cleared in this case, but that'd
require the inner recursive call to communicate to the outer recursive
call that constraint recursion occurred, likely via setting entry->result
to some sentinel value, which doesn't seem to be worth the complexity.
So this patch just relaxes the problematic assert to accept non-empty
entry->result as long as we've already issued an error.

Tested on x86_64-pc-linux-gnu, does this look OK for trunk?  Backports
seems unnecessary as the problematic assert is a checking assert.

	PR c++/100288

gcc/cp/ChangeLog:

	* constraint.cc (satisfaction_cache::get): Relax overly strict
	checking assert in the constraint recursion case.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-recursive-sat5.C: New test.
---
 gcc/cp/constraint.cc                                |  2 +-
 .../g++.dg/cpp2a/concepts-recursive-sat5.C          | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index a28c85178fe..273d15ab097 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -2705,7 +2705,7 @@ satisfaction_cache::get ()
   if (entry->evaluating)
     {
       /* If we get here, it means satisfaction is self-recursive.  */
-      gcc_checking_assert (!entry->result);
+      gcc_checking_assert (!entry->result || seen_error ());
       if (info.noisy ())
 	error_at (EXPR_LOCATION (ATOMIC_CONSTR_EXPR (entry->atom)),
 		  "satisfaction of atomic constraint %qE depends on itself",
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C
new file mode 100644
index 00000000000..b7a02815db9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C
@@ -0,0 +1,13 @@
+// PR c++/100288
+// { dg-do compile { target c++20 } }
+
+class A { };
+
+template <class T> concept pipeable = requires(A a, T t) { a | t; }; // { dg-error "depends on itself" }
+
+template <pipeable T> void operator|(A, T);
+
+void f(A tab) {
+  tab | 1; // { dg-error "no match" }
+  tab | 1; // { dg-error "no match" }
+}
-- 
2.40.0

Re: [PATCH] c++: ICE with diagnosed constraint recursion [PR100288]

[Jason Merrill]

On 3/16/23 12:48, Patrick Palka wrote:
> When satisfaction_cache::get detects constraint recursion, it asserts
> that entry->result is empty.  This makes sense when we're initially
> detecting/diagnosing recursion from the inner recursive call, but
> aftewards from the outer recursive call the recursion error is treated
> like any other SFINAE error encountered during satisfaction, and we set
> entry->result to whatever the satisfaction value ended up being.
> 
> Perhaps we should keep entry->result cleared in this case, but that'd
> require the inner recursive call to communicate to the outer recursive
> call that constraint recursion occurred, likely via setting entry->result
> to some sentinel value, which doesn't seem to be worth the complexity.
> So this patch just relaxes the problematic assert to accept non-empty
> entry->result as long as we've already issued an error.
> 
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk?  Backports
> seems unnecessary as the problematic assert is a checking assert.

OK.

> 	PR c++/100288
> 
> gcc/cp/ChangeLog:
> 
> 	* constraint.cc (satisfaction_cache::get): Relax overly strict
> 	checking assert in the constraint recursion case.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-recursive-sat5.C: New test.
> ---
>   gcc/cp/constraint.cc                                |  2 +-
>   .../g++.dg/cpp2a/concepts-recursive-sat5.C          | 13 +++++++++++++
>   2 files changed, 14 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C
> 
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index a28c85178fe..273d15ab097 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -2705,7 +2705,7 @@ satisfaction_cache::get ()
>     if (entry->evaluating)
>       {
>         /* If we get here, it means satisfaction is self-recursive.  */
> -      gcc_checking_assert (!entry->result);
> +      gcc_checking_assert (!entry->result || seen_error ());
>         if (info.noisy ())
>   	error_at (EXPR_LOCATION (ATOMIC_CONSTR_EXPR (entry->atom)),
>   		  "satisfaction of atomic constraint %qE depends on itself",
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C
> new file mode 100644
> index 00000000000..b7a02815db9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-recursive-sat5.C
> @@ -0,0 +1,13 @@
> +// PR c++/100288
> +// { dg-do compile { target c++20 } }
> +
> +class A { };
> +
> +template <class T> concept pipeable = requires(A a, T t) { a | t; }; // { dg-error "depends on itself" }
> +
> +template <pipeable T> void operator|(A, T);
> +
> +void f(A tab) {
> +  tab | 1; // { dg-error "no match" }
> +  tab | 1; // { dg-error "no match" }
> +}