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

[Jason Merrill]

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);
> +}