[constexpr,coroutines] Generic lambda coroutines cannot be constexpr [PR96251].

Hi,

PR96251 (and dups) is a rejects-valid bug against coroutines (but I’m
not sure that the problem is really there).

  * coroutines cannot be constexpr.

* Part of the way that the coroutine implementation works is a
consequence of the "lazy discovery of coroutine-ness”;  whenever
we first encounter a coroutine keyword...

.. we mark the function as a coroutine, and then we can deal with
diagnostics etc. that change under these circumstances.  This
marking of the function is independent of whether keyword
expressions are type-dependent or not.

* when we encounter a coroutine keyword in a constexpr context
we error.

* the constexpr machinery also has been taught that coroutine
expressions should cause constexpr-ness to be rejected when
checking for "potentially constexpr".

So why is there a problem?

* lambdas are implicitly constexpr from C++17.

* the constexpr machinery tends to bail early *with a conservative
   assumption that the expression is potentially constexpr* when it
   finds a type-dependent expression - without evaluating sub-
   expressions to see if they are valid (thus missing coroutine exprs.
   in such positions).

The combination of these ^^ two things, means that for many generic
lambdas with non-trivial bodies - we then enter instantiation with a
constexpr type-dependent coroutine (which is a Thing that Should not
Be).  As soon as we try to tsub any coroutine keyword expression
encountered, we error out.

* I was not able to see any way in which the instantiation process
   could be made to bail in this case and re-try for non-constexpr.

* Nor able to see somewhere else where that decision could be made.

^^ these two points reflect that I'm not familiar with the constexpr
     machinery.

* Proposed solution.

Since coroutines cannot be constexpr (not even sure what that would
mean in any practicable implementation).  The fix proposed is to
reject constexpr-ness for coroutine functions as early as possible.

* a) We can prevent a generic lambda coroutine from being converted
   to constexpr in finish_function ().  Likewise, via the tests below, we can
   avoid it for regular lambdas.
 
   b) We can also reject coroutine function decls early in both
   is_valid_constexpr_fn () and potential_constant_expression_1 ().

So - is there some alternate solution that would be better?

====

(in some ways it seems pointless to delay rejection of a coroutine
  function to some later point).

OTOH, I suppose that one could have some weird code where coroutine
coroutine keywords only appeared in a non-constexpr paths of the code
- but our current lowering is not prepared for such a circumstance.

AFAIU the standard, there's no dispensation from the "cannot be
constexpr" for such a code construction .. but ICBW.

In any event, the cases reported (and fixed by this patch) are not trying
anything so fiendishly clever.

Tested on x86_64-darwin.

Suggestions?
OK for master (after wider testing)?
thanks
Iain

= ----

coroutines cannot be constexpr, but generic lambdas (from C++17)
are implicitly assumed to be, and the processing of type-
dependent lambda bodies often terminates before any coroutine
expressions are encountered.  For example, the PR notes cases
where the coro expressions are hidden by type-dependent for or
switch expressions.

The solution proposed is to deny coroutine generic lambdas.  We also
tighten up the checks for is_valid_constexpr_fn() and do an early
test for coroutine functions in checking for potential constant
expressions.

gcc/cp/ChangeLog:

	PR c++/96251
	* constexpr.c (is_valid_constexpr_fn): Test for a
	coroutine before the chekc for lambda.
	(potential_constant_expression_1): Disallow coroutine
	function decls.
	* decl.c (finish_function): Do not mark generic
	coroutine lambda templates as constexpr.

gcc/testsuite/ChangeLog:

	PR c++/96251
	* g++.dg/coroutines/pr96251.C: New test.
---
  gcc/cp/constexpr.c                        |  7 ++++++-
  gcc/cp/decl.c                             |  2 +-
  gcc/testsuite/g++.dg/coroutines/pr96251.C | 22 ++++++++++++++++++++++
  3 files changed, 29 insertions(+), 2 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96251.C

On 2/22/21 3:59 PM, Iain Sandoe wrote:
> Hi,
> 
> PR96251 (and dups) is a rejects-valid bug against coroutines (but I’m
> not sure that the problem is really there).
> 
>   * coroutines cannot be constexpr.
> 
> * Part of the way that the coroutine implementation works is a
> consequence of the "lazy discovery of coroutine-ness”;  whenever
> we first encounter a coroutine keyword...
> 
> .. we mark the function as a coroutine, and then we can deal with
> diagnostics etc. that change under these circumstances.  This
> marking of the function is independent of whether keyword
> expressions are type-dependent or not.
> 
> * when we encounter a coroutine keyword in a constexpr context
> we error.
> 
> * the constexpr machinery also has been taught that coroutine
> expressions should cause constexpr-ness to be rejected when
> checking for "potentially constexpr".
> 
> So why is there a problem?
> 
> * lambdas are implicitly constexpr from C++17.
> 
> * the constexpr machinery tends to bail early *with a conservative
>    assumption that the expression is potentially constexpr* when it
>    finds a type-dependent expression - without evaluating sub-
>    expressions to see if they are valid (thus missing coroutine exprs.
>    in such positions).
> 
> The combination of these ^^ two things, means that for many generic
> lambdas with non-trivial bodies - we then enter instantiation with a
> constexpr type-dependent coroutine (which is a Thing that Should not
> Be).  As soon as we try to tsub any coroutine keyword expression
> encountered, we error out.
> 
> * I was not able to see any way in which the instantiation process
>    could be made to bail in this case and re-try for non-constexpr.

Many of the other places that set cp_function_chain->invalid_constexpr 
condition their errors on !is_instantiation_of_constexpr, which should 
also fix this testcase.

> * Nor able to see somewhere else where that decision could be made.
> 
> ^^ these two points reflect that I'm not familiar with the constexpr
>      machinery.
> 
> * Proposed solution.
> 
> Since coroutines cannot be constexpr (not even sure what that would
> mean in any practicable implementation).  The fix proposed is to
> reject constexpr-ness for coroutine functions as early as possible.
> 
> * a) We can prevent a generic lambda coroutine from being converted
>    to constexpr in finish_function ().  Likewise, via the tests below, 
> we can
>    avoid it for regular lambdas.
> 
>    b) We can also reject coroutine function decls early in both
>    is_valid_constexpr_fn () and potential_constant_expression_1 ().
> 
> So - is there some alternate solution that would be better?
> 
> ====
> 
> (in some ways it seems pointless to delay rejection of a coroutine
>   function to some later point).
> 
> OTOH, I suppose that one could have some weird code where coroutine
> coroutine keywords only appeared in a non-constexpr paths of the code
> - but our current lowering is not prepared for such a circumstance.
> 
> AFAIU the standard, there's no dispensation from the "cannot be
> constexpr" for such a code construction .. but ICBW.
> 
> In any event, the cases reported (and fixed by this patch) are not trying
> anything so fiendishly clever.
> 
> Tested on x86_64-darwin.
> 
> Suggestions?

> OK for master (after wider testing)?
> thanks
> Iain
> 
> = ----
> 
> coroutines cannot be constexpr, but generic lambdas (from C++17)
> are implicitly assumed to be, and the processing of type-
> dependent lambda bodies often terminates before any coroutine
> expressions are encountered.  For example, the PR notes cases
> where the coro expressions are hidden by type-dependent for or
> switch expressions.
> 
> The solution proposed is to deny coroutine generic lambdas.  We also
> tighten up the checks for is_valid_constexpr_fn() and do an early
> test for coroutine functions in checking for potential constant
> expressions.
> 
> gcc/cp/ChangeLog:
> 
>      PR c++/96251
>      * constexpr.c (is_valid_constexpr_fn): Test for a
>      coroutine before the chekc for lambda.
>      (potential_constant_expression_1): Disallow coroutine
>      function decls.
>      * decl.c (finish_function): Do not mark generic
>      coroutine lambda templates as constexpr.
> 
> gcc/testsuite/ChangeLog:
> 
>      PR c++/96251
>      * g++.dg/coroutines/pr96251.C: New test.
> ---
>   gcc/cp/constexpr.c                        |  7 ++++++-
>   gcc/cp/decl.c                             |  2 +-
>   gcc/testsuite/g++.dg/coroutines/pr96251.C | 22 ++++++++++++++++++++++
>   3 files changed, 29 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96251.C
> 
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 377fe322ee8..036bf04efa9 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -210,7 +210,9 @@ is_valid_constexpr_fn (tree fun, bool complain)
>         }
>       }
> 
> -  if (LAMBDA_TYPE_P (CP_DECL_CONTEXT (fun)) && cxx_dialect < cxx17)
> +  if (DECL_COROUTINE_P (fun))
> +    ret = false;
> +  else if (LAMBDA_TYPE_P (CP_DECL_CONTEXT (fun)) && cxx_dialect < cxx17)
>       {
>         ret = false;
>         if (complain)
> @@ -7827,6 +7829,9 @@ potential_constant_expression_1 (tree t, bool 
> want_rval, bool strict, bool now,
>     switch (TREE_CODE (t))
>       {
>       case FUNCTION_DECL:
> +      if (DECL_COROUTINE_P (t))
> +    return false;
> +    /* FALLTHROUGH.  */
>       case BASELINK:
>       case TEMPLATE_DECL:
>       case OVERLOAD:
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 7fa8f52d667..3a089b5bee5 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -17374,7 +17374,7 @@ finish_function (bool inline_p)
>     if (cxx_dialect >= cxx17
>         && LAMBDA_TYPE_P (CP_DECL_CONTEXT (fndecl)))
>       DECL_DECLARED_CONSTEXPR_P (fndecl)
> -      = ((processing_template_decl
> +      = (((processing_template_decl && !DECL_COROUTINE_P(fndecl))
>         || is_valid_constexpr_fn (fndecl, /*complain*/false))
>        && potential_constant_expression (DECL_SAVED_TREE (fndecl)));
> 
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr96251.C 
> b/gcc/testsuite/g++.dg/coroutines/pr96251.C
> new file mode 100644
> index 00000000000..6824d783d5f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr96251.C
> @@ -0,0 +1,22 @@
> +#include <coroutine>
> +
> +struct coroutine {
> +  struct promise_type {
> +    auto get_return_object() { return coroutine(); }
> +    auto initial_suspend() { return std::suspend_always(); }
> +    auto yield_value(int) { return std::suspend_always(); }
> +    void return_void() {}
> +    auto final_suspend() noexcept { return std::suspend_always(); }
> +    void unhandled_exception() {}
> +  };
> +};
> +
> +int main() {
> +  auto f = [](auto max) -> coroutine {
> +    for (int i = 0; i < max; ++i) {
> +       co_yield i;
> +    }
> +  };
> +
> +  f(10);
> +}

Hi Jason,

Jason Merrill <jason@redhat.com> wrote:

> On 2/22/21 3:59 PM, Iain Sandoe wrote:

>> * I was not able to see any way in which the instantiation process
>>   could be made to bail in this case and re-try for non-constexpr.
> 
> Many of the other places that set cp_function_chain->invalid_constexpr condition their errors on !is_instantiation_of_constexpr, which should also fix this testcase.

Thanks!
(FWIW, there only seem to be three instances of this in the FE and two of those are in constexpr.c).

so like this?
(tested on x86_64-darwin, regtest running x86_64 linux)

thanks
iain

====
 [PATCH] coroutines : Adjust error handling for type-dependent coroutines [PR96251].

Although coroutines are not permitted to be constexpr, generic lambdas
are implicitly from C++17 and, because of this, a generic coroutine lambda
can be marked as potentially constexpr. As per the PR, this then fails when
type substitution is attempted because the check disallowing constexpr in
the coroutines code was overly restrictive.

This changes the error handing to mark the function  as 'invalid_constexpr'
but suppresses the error in the case that we are instantiating a constexpr.

gcc/cp/ChangeLog:

	PR c++/PR96251
	* coroutines.cc (coro_common_keyword_context_valid_p): Suppress
	error reporting when instantiating for a constexpr.

gcc/testsuite/ChangeLog:

	PR c++/96251
	* g++.dg/coroutines/pr96251.C: New test.
---
 gcc/cp/coroutines.cc                      | 11 +++++---
 gcc/testsuite/g++.dg/coroutines/pr96251.C | 32 +++++++++++++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96251.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index e61de1fac01..abfe8d08192 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -683,11 +683,14 @@ coro_common_keyword_context_valid_p (tree fndecl, location_t kw_loc,
 
   if (DECL_DECLARED_CONSTEXPR_P (fndecl))
     {
-      /* [dcl.constexpr] 3.3 it shall not be a coroutine.  */
-      error_at (kw_loc, "%qs cannot be used in a %<constexpr%> function",
-		kw_name);
       cp_function_chain->invalid_constexpr = true;
-      return false;
+      if (!is_instantiation_of_constexpr (fndecl))
+	{
+	  /* [dcl.constexpr] 3.3 it shall not be a coroutine.  */
+	  error_at (kw_loc, "%qs cannot be used in a %<constexpr%> function",
+		    kw_name);
+	  return false;
+	}
     }
 
   if (FNDECL_USED_AUTO (fndecl))
diff --git a/gcc/testsuite/g++.dg/coroutines/pr96251.C b/gcc/testsuite/g++.dg/coroutines/pr96251.C
new file mode 100644
index 00000000000..3f435044e41
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr96251.C
@@ -0,0 +1,32 @@
+#include <coroutine>
+    
+struct coroutine {
+  struct promise_type {
+    auto get_return_object() { return coroutine(); }
+    auto initial_suspend() { return std::suspend_always(); }
+    auto yield_value(int) { return std::suspend_always(); }
+    void return_void() {}
+    auto final_suspend() noexcept { return std::suspend_always(); }
+    void unhandled_exception() {}
+  };
+};
+
+int main() {
+  auto f = [](auto max) -> coroutine {
+    for (int i = 0; i < max; ++i) {
+       co_yield i;
+    }
+  };
+
+  f(10);
+
+  // From PR98976
+  auto foo = [](auto&&) -> coroutine {
+    switch (42) {
+      case 42:
+        co_return;
+    }
+  };
+  foo(1);
+
+}

On 2/23/21 8:20 AM, Iain Sandoe wrote:
> Hi Jason,
> 
> Jason Merrill <jason@redhat.com> wrote:
> 
>> On 2/22/21 3:59 PM, Iain Sandoe wrote:
> 
>>> * I was not able to see any way in which the instantiation process
>>>    could be made to bail in this case and re-try for non-constexpr.
>>
>> Many of the other places that set cp_function_chain->invalid_constexpr condition their errors on !is_instantiation_of_constexpr, which should also fix this testcase.
> 
> Thanks!
> (FWIW, there only seem to be three instances of this in the FE and two of those are in constexpr.c).
> 
> so like this?
> (tested on x86_64-darwin, regtest running x86_64 linux)

OK.

> thanks
> iain
> 
> ====
>   [PATCH] coroutines : Adjust error handling for type-dependent coroutines [PR96251].
> 
> Although coroutines are not permitted to be constexpr, generic lambdas
> are implicitly from C++17 and, because of this, a generic coroutine lambda
> can be marked as potentially constexpr. As per the PR, this then fails when
> type substitution is attempted because the check disallowing constexpr in
> the coroutines code was overly restrictive.
> 
> This changes the error handing to mark the function  as 'invalid_constexpr'
> but suppresses the error in the case that we are instantiating a constexpr.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/PR96251
> 	* coroutines.cc (coro_common_keyword_context_valid_p): Suppress
> 	error reporting when instantiating for a constexpr.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/96251
> 	* g++.dg/coroutines/pr96251.C: New test.
> ---
>   gcc/cp/coroutines.cc                      | 11 +++++---
>   gcc/testsuite/g++.dg/coroutines/pr96251.C | 32 +++++++++++++++++++++++
>   2 files changed, 39 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96251.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index e61de1fac01..abfe8d08192 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -683,11 +683,14 @@ coro_common_keyword_context_valid_p (tree fndecl, location_t kw_loc,
>   
>     if (DECL_DECLARED_CONSTEXPR_P (fndecl))
>       {
> -      /* [dcl.constexpr] 3.3 it shall not be a coroutine.  */
> -      error_at (kw_loc, "%qs cannot be used in a %<constexpr%> function",
> -		kw_name);
>         cp_function_chain->invalid_constexpr = true;
> -      return false;
> +      if (!is_instantiation_of_constexpr (fndecl))
> +	{
> +	  /* [dcl.constexpr] 3.3 it shall not be a coroutine.  */
> +	  error_at (kw_loc, "%qs cannot be used in a %<constexpr%> function",
> +		    kw_name);
> +	  return false;
> +	}
>       }
>   
>     if (FNDECL_USED_AUTO (fndecl))
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr96251.C b/gcc/testsuite/g++.dg/coroutines/pr96251.C
> new file mode 100644
> index 00000000000..3f435044e41
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr96251.C
> @@ -0,0 +1,32 @@
> +#include <coroutine>
> +
> +struct coroutine {
> +  struct promise_type {
> +    auto get_return_object() { return coroutine(); }
> +    auto initial_suspend() { return std::suspend_always(); }
> +    auto yield_value(int) { return std::suspend_always(); }
> +    void return_void() {}
> +    auto final_suspend() noexcept { return std::suspend_always(); }
> +    void unhandled_exception() {}
> +  };
> +};
> +
> +int main() {
> +  auto f = [](auto max) -> coroutine {
> +    for (int i = 0; i < max; ++i) {
> +       co_yield i;
> +    }
> +  };
> +
> +  f(10);
> +
> +  // From PR98976
> +  auto foo = [](auto&&) -> coroutine {
> +    switch (42) {
> +      case 42:
> +        co_return;
> +    }
> +  };
> +  foo(1);
> +
> +}
>