c++, coroutines: Account for overloaded promise return_value() [PR105301].

Whether it was intended or not, it is possible to define a coroutine promise
with multiple return_value() methods [which need not even have the same type].

We were not accounting for this possibility in the check to see whether both
return_value and return_void are specifier (which is prohibited by the
standard).  Fixed thus and provided an adjusted diagnostic for the case that
multiple return_value() methods are present.

tested on x86_64-darwin, OK for mainline? / Backports? (when?)
thanks,
Iain

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

	PR c++/105301

gcc/cp/ChangeLog:

	* coroutines.cc (coro_promise_type_found_p): Account for possible
	mutliple overloads of the promise return_value() method.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr105301.C: New test.
---
 gcc/cp/coroutines.cc                       | 10 ++++-
 gcc/testsuite/g++.dg/coroutines/pr105301.C | 49 ++++++++++++++++++++++
 2 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105301.C

On 4/18/22 10:03, Iain Sandoe wrote:
> Whether it was intended or not, it is possible to define a coroutine promise
> with multiple return_value() methods [which need not even have the same type].
> 
> We were not accounting for this possibility in the check to see whether both
> return_value and return_void are specifier (which is prohibited by the
> standard).  Fixed thus and provided an adjusted diagnostic for the case that
> multiple return_value() methods are present.
> 
> tested on x86_64-darwin, OK for mainline? / Backports? (when?)
> thanks,
> Iain
> 
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/105301
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (coro_promise_type_found_p): Account for possible
> 	mutliple overloads of the promise return_value() method.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr105301.C: New test.
> ---
>   gcc/cp/coroutines.cc                       | 10 ++++-
>   gcc/testsuite/g++.dg/coroutines/pr105301.C | 49 ++++++++++++++++++++++
>   2 files changed, 57 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105301.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index dcc2284171b..d2a765cac11 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -513,8 +513,14 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
>   		      coro_info->promise_type);
>   	  inform (DECL_SOURCE_LOCATION (BASELINK_FUNCTIONS (has_ret_void)),
>   		  "%<return_void%> declared here");
> -	  inform (DECL_SOURCE_LOCATION (BASELINK_FUNCTIONS (has_ret_val)),
> -		  "%<return_value%> declared here");
> +	  has_ret_val = BASELINK_FUNCTIONS (has_ret_val);
> +	  const char *message = "%<return_value%> declared here";
> +	  if (TREE_CODE (has_ret_val) == OVERLOAD)
> +	    {
> +	      has_ret_val = OVL_FIRST (has_ret_val);
> +	      message = "%<return_value%> first declared here";
> +	    }

You could also use get_first_fn, but the patch is OK as is.  I'm 
inclined to leave backports in coroutines.cc to your discretion, you 
probably have a better idea of how important they are.

> +	  inform (DECL_SOURCE_LOCATION (has_ret_val), message);
>   	  coro_info->coro_co_return_error_emitted = true;
>   	  return false;
>   	}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr105301.C b/gcc/testsuite/g++.dg/coroutines/pr105301.C
> new file mode 100644
> index 00000000000..33a0b03cf5d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr105301.C
> @@ -0,0 +1,49 @@
> +// { dg-additional-options "-fsyntax-only" }
> +namespace std {
> +template <class T, class = void>
> +struct traits_sfinae_base {};
> +
> +template <class Ret, class... Args>
> +struct coroutine_traits : public traits_sfinae_base<Ret> {};
> +}
> +
> +template<typename Promise> struct coro {};
> +template <typename Promise, typename... Ps>
> +struct std::coroutine_traits<coro<Promise>, Ps...> {
> +  using promise_type = Promise;
> +};
> +
> +struct awaitable {
> +  bool await_ready() noexcept;
> +  template <typename F>
> +  void await_suspend(F) noexcept;
> +  void await_resume() noexcept;
> +} a;
> +
> +struct suspend_always {
> +  bool await_ready() noexcept { return false; }
> +  template <typename F>
> +  void await_suspend(F) noexcept;
> +  void await_resume() noexcept {}
> +};
> +
> +namespace std {
> +template <class PromiseType = void>
> +struct coroutine_handle {};
> +}
> +
> +struct bad_promise_6 {
> +  coro<bad_promise_6> get_return_object();
> +  suspend_always initial_suspend();
> +  suspend_always final_suspend() noexcept;
> +  void unhandled_exception();
> +  void return_void();
> +  void return_value(int) const;
> +  void return_value(int);
> +};
> +
> +coro<bad_promise_6>
> +bad_implicit_return() // { dg-error {.aka 'bad_promise_6'. declares both 'return_value' and 'return_void'} }
> +{
> +  co_await a;
> +}

On Wed, Apr 20, 2022 at 4:19 AM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 4/18/22 10:03, Iain Sandoe wrote:
> > Whether it was intended or not, it is possible to define a coroutine promise
> > with multiple return_value() methods [which need not even have the same type].
> >
> > We were not accounting for this possibility in the check to see whether both
> > return_value and return_void are specifier (which is prohibited by the
> > standard).  Fixed thus and provided an adjusted diagnostic for the case that
> > multiple return_value() methods are present.
> >
> > tested on x86_64-darwin, OK for mainline? / Backports? (when?)
> > thanks,
> > Iain
> >
> > Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> >
> >       PR c++/105301
> >
> > gcc/cp/ChangeLog:
> >
> >       * coroutines.cc (coro_promise_type_found_p): Account for possible
> >       mutliple overloads of the promise return_value() method.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * g++.dg/coroutines/pr105301.C: New test.
> > ---
> >   gcc/cp/coroutines.cc                       | 10 ++++-
> >   gcc/testsuite/g++.dg/coroutines/pr105301.C | 49 ++++++++++++++++++++++
> >   2 files changed, 57 insertions(+), 2 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr105301.C
> >
> > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> > index dcc2284171b..d2a765cac11 100644
> > --- a/gcc/cp/coroutines.cc
> > +++ b/gcc/cp/coroutines.cc
> > @@ -513,8 +513,14 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
> >                     coro_info->promise_type);
> >         inform (DECL_SOURCE_LOCATION (BASELINK_FUNCTIONS (has_ret_void)),
> >                 "%<return_void%> declared here");
> > -       inform (DECL_SOURCE_LOCATION (BASELINK_FUNCTIONS (has_ret_val)),
> > -               "%<return_value%> declared here");
> > +       has_ret_val = BASELINK_FUNCTIONS (has_ret_val);
> > +       const char *message = "%<return_value%> declared here";
> > +       if (TREE_CODE (has_ret_val) == OVERLOAD)
> > +         {
> > +           has_ret_val = OVL_FIRST (has_ret_val);
> > +           message = "%<return_value%> first declared here";
> > +         }
>
> You could also use get_first_fn, but the patch is OK as is.  I'm
> inclined to leave backports in coroutines.cc to your discretion, you
> probably have a better idea of how important they are.

Likewise.  Please wait until after the 11.3 release.

Richard.

> > +       inform (DECL_SOURCE_LOCATION (has_ret_val), message);
> >         coro_info->coro_co_return_error_emitted = true;
> >         return false;
> >       }
> > diff --git a/gcc/testsuite/g++.dg/coroutines/pr105301.C b/gcc/testsuite/g++.dg/coroutines/pr105301.C
> > new file mode 100644
> > index 00000000000..33a0b03cf5d
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/coroutines/pr105301.C
> > @@ -0,0 +1,49 @@
> > +// { dg-additional-options "-fsyntax-only" }
> > +namespace std {
> > +template <class T, class = void>
> > +struct traits_sfinae_base {};
> > +
> > +template <class Ret, class... Args>
> > +struct coroutine_traits : public traits_sfinae_base<Ret> {};
> > +}
> > +
> > +template<typename Promise> struct coro {};
> > +template <typename Promise, typename... Ps>
> > +struct std::coroutine_traits<coro<Promise>, Ps...> {
> > +  using promise_type = Promise;
> > +};
> > +
> > +struct awaitable {
> > +  bool await_ready() noexcept;
> > +  template <typename F>
> > +  void await_suspend(F) noexcept;
> > +  void await_resume() noexcept;
> > +} a;
> > +
> > +struct suspend_always {
> > +  bool await_ready() noexcept { return false; }
> > +  template <typename F>
> > +  void await_suspend(F) noexcept;
> > +  void await_resume() noexcept {}
> > +};
> > +
> > +namespace std {
> > +template <class PromiseType = void>
> > +struct coroutine_handle {};
> > +}
> > +
> > +struct bad_promise_6 {
> > +  coro<bad_promise_6> get_return_object();
> > +  suspend_always initial_suspend();
> > +  suspend_always final_suspend() noexcept;
> > +  void unhandled_exception();
> > +  void return_void();
> > +  void return_value(int) const;
> > +  void return_value(int);
> > +};
> > +
> > +coro<bad_promise_6>
> > +bad_implicit_return() // { dg-error {.aka 'bad_promise_6'. declares both 'return_value' and 'return_void'} }
> > +{
> > +  co_await a;
> > +}
>