c++: improve diagnostic of 'return's in coroutines

Enlargening the function-specific data block is not great.  I've
considered changing the location of RETURN_STMT expressions to cover
everything from the return expression to input_location after parsing
the returned expr.  The result of that is:

test.cc:38:3: error: a ‘return’ statement is not allowed in coroutine; did you mean ‘co_return’?
   38 |   return {};
      |   ^~~~~~~~~
test.cc:37:3: note: function was made a coroutine here
   37 |   co_return;
      |   ^~~~~~~~~

... so, not bad, but I'm not sure how intrusive such a change would be
(haven't tried the testsuite).  The current patch produces:

test.cc:36:3: error: a ‘return’ statement is not allowed in coroutine; did you mean ‘co_return’?
   36 |   return {};
      |   ^~~~~~
test.cc:35:3: note: function was made a coroutine here
   35 |   co_return;
      |   ^~~~~~~~~


Is there a better location to use here or is the current (latter) one
OK?  I haven't managed to found a nicer existing one.  We also can't
stash it in coroutine_info because a function might not have that at
time we parse a return.

Tested on x86_64-pc-linux-gnu.

Have a lovely evening.
---------- >8 ----------
We now point out why a function is a coroutine.

gcc/cp/ChangeLog:

	* coroutines.cc (coro_function_valid_p): Change how we diagnose
	returning coroutines.
	* cp-tree.h (struct language_function): Add first_return_loc
	field.  Tracks the location of the first return encountered
	during parsing.
	(current_function_first_return_loc): New macro.  Expands to the
	current functions' first_return_loc.
	* parser.cc (cp_parser_jump_statement): If parsing a RID_RETURN,
	save its location to current_function_first_return_loc.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to
	match new diagnostic.
---
 gcc/cp/coroutines.cc                          | 14 +++--
 gcc/cp/cp-tree.h                              |  6 +++
 gcc/cp/parser.cc                              |  4 ++
 .../co-return-syntax-08-bad-return.C          | 52 +++++++++++++++++--
 4 files changed, 68 insertions(+), 8 deletions(-)

On 8/7/24 7:31 PM, Arsen Arsenović wrote:
> Enlargening the function-specific data block is not great.

Indeed, I think it would be better to search DECL_SAVED_TREE for a 
RETURN_STMT once we've decided to give an error.

> I've
> considered changing the location of RETURN_STMT expressions to cover
> everything from the return expression to input_location after parsing
> the returned expr.  The result of that is:
> 
> test.cc:38:3: error: a ‘return’ statement is not allowed in coroutine; did you mean ‘co_return’?
>     38 |   return {};
>        |   ^~~~~~~~~
> test.cc:37:3: note: function was made a coroutine here
>     37 |   co_return;
>        |   ^~~~~~~~~
> 
> ... so, not bad, but I'm not sure how intrusive such a change would be
> (haven't tried the testsuite).  The current patch produces:
> 
> test.cc:36:3: error: a ‘return’ statement is not allowed in coroutine; did you mean ‘co_return’?
>     36 |   return {};
>        |   ^~~~~~
> test.cc:35:3: note: function was made a coroutine here
>     35 |   co_return;
>        |   ^~~~~~~~~
> 
> 
> Is there a better location to use here or is the current (latter) one
> OK?

The latter seems fine.

> I haven't managed to found a nicer existing one.  We also can't
> stash it in coroutine_info because a function might not have that at
> time we parse a return.
> 
> Tested on x86_64-pc-linux-gnu.
> 
> Have a lovely evening.
> ---------- >8 ----------
> We now point out why a function is a coroutine.
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (coro_function_valid_p): Change how we diagnose
> 	returning coroutines.
> 	* cp-tree.h (struct language_function): Add first_return_loc
> 	field.  Tracks the location of the first return encountered
> 	during parsing.
> 	(current_function_first_return_loc): New macro.  Expands to the
> 	current functions' first_return_loc.
> 	* parser.cc (cp_parser_jump_statement): If parsing a RID_RETURN,
> 	save its location to current_function_first_return_loc.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to
> 	match new diagnostic.
> ---
>   gcc/cp/coroutines.cc                          | 14 +++--
>   gcc/cp/cp-tree.h                              |  6 +++
>   gcc/cp/parser.cc                              |  4 ++
>   .../co-return-syntax-08-bad-return.C          | 52 +++++++++++++++++--
>   4 files changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 0f4dc42ec1c8..f32c7a2eec8d 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -968,11 +968,15 @@ coro_function_valid_p (tree fndecl)
>   
>     if (current_function_returns_value || current_function_returns_null)
>       {
> -       /* TODO: record or extract positions of returns (and the first coro
> -	  keyword) so that we can add notes to the diagnostic about where
> -	  the bad keyword is and what made the function into a coro.  */
> -      error_at (f_loc, "a %<return%> statement is not allowed in coroutine;"
> -			" did you mean %<co_return%>?");
> +      coroutine_info *coro_info = get_or_insert_coroutine_info (fndecl);
> +      auto retloc = current_function_first_return_loc;
> +      gcc_checking_assert (retloc && coro_info->first_coro_keyword);
> +
> +      auto_diagnostic_group diaggrp;
> +      error_at (retloc, "a %<return%> statement is not allowed in coroutine;"
> +		" did you mean %<co_return%>?");
> +      inform (coro_info->first_coro_keyword,
> +	      "function was made a coroutine here");
>         return false;
>       }
>   
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 911d1d7924cc..68c681150a1f 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -2123,6 +2123,8 @@ struct GTY(()) language_function {
>     tree x_vtt_parm;
>     tree x_return_value;
>   
> +  location_t first_return_loc;
> +
>     BOOL_BITFIELD returns_value : 1;
>     BOOL_BITFIELD returns_null : 1;
>     BOOL_BITFIELD returns_abnormally : 1;
> @@ -2217,6 +2219,10 @@ struct GTY(()) language_function {
>   #define current_function_return_value \
>     (cp_function_chain->x_return_value)
>   
> +/* Location of the first 'return' stumbled upon during parsing.  */
> +
> +#define current_function_first_return_loc cp_function_chain->first_return_loc
> +
>   /* In parser.cc.  */
>   extern tree cp_literal_operator_id (const char *);
>   
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index eb102dea8299..6cfe42f3bdd6 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -14957,6 +14957,10 @@ cp_parser_jump_statement (cp_parser* parser, tree &std_attrs)
>   	      AGGR_INIT_EXPR_MUST_TAIL (ret_expr) = musttail_p;
>   	    else
>   	      set_musttail_on_return (expr, token->location, musttail_p);
> +
> +	    /* Save where we saw this keyword.  */
> +	    if (current_function_first_return_loc == UNKNOWN_LOCATION)
> +	      current_function_first_return_loc = token->location;
>   	  }
>   
>   	/* Build the return-statement, check co-return first, since type
> diff --git a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
> index 148ee4543e87..1e5d9e7a65a1 100644
> --- a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
> +++ b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
> @@ -27,6 +27,7 @@ struct Coro {
>     auto final_suspend () noexcept { return coro::suspend_always{}; }
>     void return_void () { }
>      void unhandled_exception() { }
> +  auto yield_value (auto) noexcept { return coro::suspend_always{}; }
>     };
>   };
>   
> @@ -34,10 +35,55 @@ extern int x;
>   
>   // Diagnose disallowed "return" in coroutine.
>   Coro
> -bar () // { dg-error {a 'return' statement is not allowed} }
> +bar ()
>   {
>     if (x)
> -    return Coro();
> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>     else
> -    co_return;
> +    co_return; // { dg-note "function was made a coroutine here" }
> +}
> +
> +Coro
> +bar1 ()
> +{
> +  if (x)
> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
> +  else
> +    co_await std::suspend_never{}; // { dg-note "function was made a coroutine here" }
> +}
> +
> +Coro
> +bar2 ()
> +{
> +  if (x)
> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
> +  else
> +    co_yield 123; // { dg-note "function was made a coroutine here" }
> +}
> +
> +Coro
> +bar3 ()
> +{
> +  if (x)
> +    co_return; // { dg-note "function was made a coroutine here" }
> +  else
> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
> +}
> +
> +Coro
> +bar4 ()
> +{
> +  if (x)
> +    co_await std::suspend_never{}; // { dg-note "function was made a coroutine here" }
> +  else
> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
> +}
> +
> +Coro
> +bar5 ()
> +{
> +  if (x)
> +    co_yield 123; // { dg-note "function was made a coroutine here" }
> +  else
> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>   }

Jason Merrill <jason@redhat.com> writes:

> On 8/7/24 7:31 PM, Arsen Arsenović wrote:
>> Enlargening the function-specific data block is not great.
>
> Indeed, I think it would be better to search DECL_SAVED_TREE for a RETURN_STMT
> once we've decided to give an error.

The trouble with that is that finish_return_stmt currently uses
input_location as the location for the entire return expr, so the
location ends up being after the entire return value.

I've hacked in a way to provide a different location to
finih_return_stmt, when applying it like below, the produced result is
the first result in the original email:

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 6cfe42f3bdd6..44b45f16b026 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -14965,12 +14965,15 @@ cp_parser_jump_statement (cp_parser* parser, tree &std_attrs)
 
 	/* Build the return-statement, check co-return first, since type
 	  deduction is not valid there.  */
+	auto l = make_location (token->location,
+				token->location,
+				input_location);
 	if (keyword == RID_CO_RETURN)
 	 statement = finish_co_return_stmt (token->location, expr);
 	else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt)
 	 /* Don't deduce from a discarded return statement.  */;
 	else
-	 statement = finish_return_stmt (expr);
+	 statement = finish_return_stmt (expr, l);
 	/* Look for the final `;'.  */
 	cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
       }

... without this change (so, using input_location), the result is:

test.cc:38:11: error: a ‘return’ statement is not allowed in coroutine; did you mean ‘co_return’?
   38 |   return {};
      |           ^

... which is not the best.  That's the change I'm referring to in the
original post that I haven't ran the testsuite on.  Changing that
location allows for simply searching DECL_SAVED_TREE (fndecl), though,
and getting a good location out of it.

>> I've
>> considered changing the location of RETURN_STMT expressions to cover
>> everything from the return expression to input_location after parsing
>> the returned expr.  The result of that is:
>> test.cc:38:3: error: a ‘return’ statement is not allowed in coroutine; did
>> you mean ‘co_return’?
>>     38 |   return {};
>>        |   ^~~~~~~~~
>> test.cc:37:3: note: function was made a coroutine here
>>     37 |   co_return;
>>        |   ^~~~~~~~~
>> ... so, not bad, but I'm not sure how intrusive such a change would be
>> (haven't tried the testsuite).  The current patch produces:
>> test.cc:36:3: error: a ‘return’ statement is not allowed in coroutine; did
>> you mean ‘co_return’?
>>     36 |   return {};
>>        |   ^~~~~~
>> test.cc:35:3: note: function was made a coroutine here
>>     35 |   co_return;
>>        |   ^~~~~~~~~
>> Is there a better location to use here or is the current (latter) one
>> OK?
>
> The latter seems fine.
>
>> I haven't managed to found a nicer existing one.  We also can't
>> stash it in coroutine_info because a function might not have that at
>> time we parse a return.
>> Tested on x86_64-pc-linux-gnu.
>> Have a lovely evening.
>> ---------- >8 ----------
>> We now point out why a function is a coroutine.
>> gcc/cp/ChangeLog:
>> 	* coroutines.cc (coro_function_valid_p): Change how we diagnose
>> 	returning coroutines.
>> 	* cp-tree.h (struct language_function): Add first_return_loc
>> 	field.  Tracks the location of the first return encountered
>> 	during parsing.
>> 	(current_function_first_return_loc): New macro.  Expands to the
>> 	current functions' first_return_loc.
>> 	* parser.cc (cp_parser_jump_statement): If parsing a RID_RETURN,
>> 	save its location to current_function_first_return_loc.
>> gcc/testsuite/ChangeLog:
>> 	* g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to
>> 	match new diagnostic.
>> ---
>>   gcc/cp/coroutines.cc                          | 14 +++--
>>   gcc/cp/cp-tree.h                              |  6 +++
>>   gcc/cp/parser.cc                              |  4 ++
>>   .../co-return-syntax-08-bad-return.C          | 52 +++++++++++++++++--
>>   4 files changed, 68 insertions(+), 8 deletions(-)
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index 0f4dc42ec1c8..f32c7a2eec8d 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -968,11 +968,15 @@ coro_function_valid_p (tree fndecl)
>>       if (current_function_returns_value || current_function_returns_null)
>>       {
>> -       /* TODO: record or extract positions of returns (and the first coro
>> -	  keyword) so that we can add notes to the diagnostic about where
>> -	  the bad keyword is and what made the function into a coro.  */
>> -      error_at (f_loc, "a %<return%> statement is not allowed in coroutine;"
>> -			" did you mean %<co_return%>?");
>> +      coroutine_info *coro_info = get_or_insert_coroutine_info (fndecl);
>> +      auto retloc = current_function_first_return_loc;
>> +      gcc_checking_assert (retloc && coro_info->first_coro_keyword);
>> +
>> +      auto_diagnostic_group diaggrp;
>> +      error_at (retloc, "a %<return%> statement is not allowed in coroutine;"
>> +		" did you mean %<co_return%>?");
>> +      inform (coro_info->first_coro_keyword,
>> +	      "function was made a coroutine here");
>>         return false;
>>       }
>>   diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>> index 911d1d7924cc..68c681150a1f 100644
>> --- a/gcc/cp/cp-tree.h
>> +++ b/gcc/cp/cp-tree.h
>> @@ -2123,6 +2123,8 @@ struct GTY(()) language_function {
>>     tree x_vtt_parm;
>>     tree x_return_value;
>>   +  location_t first_return_loc;
>> +
>>     BOOL_BITFIELD returns_value : 1;
>>     BOOL_BITFIELD returns_null : 1;
>>     BOOL_BITFIELD returns_abnormally : 1;
>> @@ -2217,6 +2219,10 @@ struct GTY(()) language_function {
>>   #define current_function_return_value \
>>     (cp_function_chain->x_return_value)
>>   +/* Location of the first 'return' stumbled upon during parsing.  */
>> +
>> +#define current_function_first_return_loc cp_function_chain->first_return_loc
>> +
>>   /* In parser.cc.  */
>>   extern tree cp_literal_operator_id (const char *);
>>   diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>> index eb102dea8299..6cfe42f3bdd6 100644
>> --- a/gcc/cp/parser.cc
>> +++ b/gcc/cp/parser.cc
>> @@ -14957,6 +14957,10 @@ cp_parser_jump_statement (cp_parser* parser, tree &std_attrs)
>>   	      AGGR_INIT_EXPR_MUST_TAIL (ret_expr) = musttail_p;
>>   	    else
>>   	      set_musttail_on_return (expr, token->location, musttail_p);
>> +
>> +	    /* Save where we saw this keyword.  */
>> +	    if (current_function_first_return_loc == UNKNOWN_LOCATION)
>> +	      current_function_first_return_loc = token->location;
>>   	  }
>>     	/* Build the return-statement, check co-return first, since type
>> diff --git a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> index 148ee4543e87..1e5d9e7a65a1 100644
>> --- a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> +++ b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> @@ -27,6 +27,7 @@ struct Coro {
>>     auto final_suspend () noexcept { return coro::suspend_always{}; }
>>     void return_void () { }
>>      void unhandled_exception() { }
>> +  auto yield_value (auto) noexcept { return coro::suspend_always{}; }
>>     };
>>   };
>>   @@ -34,10 +35,55 @@ extern int x;
>>     // Diagnose disallowed "return" in coroutine.
>>   Coro
>> -bar () // { dg-error {a 'return' statement is not allowed} }
>> +bar ()
>>   {
>>     if (x)
>> -    return Coro();
>> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>>     else
>> -    co_return;
>> +    co_return; // { dg-note "function was made a coroutine here" }
>> +}
>> +
>> +Coro
>> +bar1 ()
>> +{
>> +  if (x)
>> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>> +  else
>> +    co_await std::suspend_never{}; // { dg-note "function was made a coroutine here" }
>> +}
>> +
>> +Coro
>> +bar2 ()
>> +{
>> +  if (x)
>> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>> +  else
>> +    co_yield 123; // { dg-note "function was made a coroutine here" }
>> +}
>> +
>> +Coro
>> +bar3 ()
>> +{
>> +  if (x)
>> +    co_return; // { dg-note "function was made a coroutine here" }
>> +  else
>> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>> +}
>> +
>> +Coro
>> +bar4 ()
>> +{
>> +  if (x)
>> +    co_await std::suspend_never{}; // { dg-note "function was made a coroutine here" }
>> +  else
>> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>> +}
>> +
>> +Coro
>> +bar5 ()
>> +{
>> +  if (x)
>> +    co_yield 123; // { dg-note "function was made a coroutine here" }
>> +  else
>> +    return Coro();  // { dg-error {a 'return' statement is not allowed} }
>>   }

On 8/7/24 9:00 PM, Arsen Arsenović wrote:
> Jason Merrill <jason@redhat.com> writes:
> 
>> On 8/7/24 7:31 PM, Arsen Arsenović wrote:
>>> Enlargening the function-specific data block is not great.
>>
>> Indeed, I think it would be better to search DECL_SAVED_TREE for a RETURN_STMT
>> once we've decided to give an error.
> 
> The trouble with that is that finish_return_stmt currently uses
> input_location as the location for the entire return expr, so the
> location ends up being after the entire return value.
> 
> I've hacked in a way to provide a different location to
> finih_return_stmt, when applying it like below, the produced result is
> the first result in the original email:
> 
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 6cfe42f3bdd6..44b45f16b026 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -14965,12 +14965,15 @@ cp_parser_jump_statement (cp_parser* parser, tree &std_attrs)
>   
>   	/* Build the return-statement, check co-return first, since type
>   	  deduction is not valid there.  */
> +	auto l = make_location (token->location,
> +				token->location,
> +				input_location);
>   	if (keyword == RID_CO_RETURN)
>   	 statement = finish_co_return_stmt (token->location, expr);
>   	else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt)
>   	 /* Don't deduce from a discarded return statement.  */;
>   	else
> -	 statement = finish_return_stmt (expr);
> +	 statement = finish_return_stmt (expr, l);
>   	/* Look for the final `;'.  */
>   	cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
>         }
> 
> ... without this change (so, using input_location), the result is:
> 
> test.cc:38:11: error: a ‘return’ statement is not allowed in coroutine; did you mean ‘co_return’?
>     38 |   return {};
>        |           ^
> 
> ... which is not the best.  That's the change I'm referring to in the
> original post that I haven't ran the testsuite on.  Changing that
> location allows for simply searching DECL_SAVED_TREE (fndecl), though,
> and getting a good location out of it.

Sounds good.

Jason