diff mbox series

[v3,2/2] c++: improve diagnostic of 'return's in coroutines

Message ID 20240821180101.3976132-2-arsen@aarsen.me
State New
Headers show
Series [v3,1/2] c++: improve location of parsed RETURN_EXPRs | expand

Commit Message

Arsen Arsenović Aug. 21, 2024, 5:52 p.m. UTC
We now point out why a function is a coroutine, and where (the first
return) is in the function.

gcc/cp/ChangeLog:

	* coroutines.cc (struct coroutine_info): Rename
	first_coro_keyword -> first_coro_expr.  The former name is no
	longer accurate.
	(coro_promise_type_found_p): Adjust accordingly.
	(coro_function_valid_p): Change how we diagnose 'return'
	statements in coroutines to also point out where a function was
	made a coroutine, and where 'return' was used.
	(find_coro_traits_template_decl): Rename kw parameter into loc,
	since it might not refer to a keyword always.
	(instantiate_coro_traits): Ditto.
	(find_coro_handle_template_decl): Ditto.
	(get_handle_type_address): Ditto.
	(get_handle_type_from_address): Ditto.
	(instantiate_coro_handle_for_promise_type): Ditto.
	(build_template_co_await_expr): Ditto.
	(finish_co_await_expr): Ditto.
	(finish_co_yield_expr): Ditto.
	(finish_co_return_stmt): Ditto.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to
	match new diagnostic.  Test more keyword combinations.
---
 gcc/cp/coroutines.cc                          | 127 ++++++++++--------
 .../co-return-syntax-08-bad-return.C          |  52 ++++++-
 2 files changed, 119 insertions(+), 60 deletions(-)

Comments

Jason Merrill Aug. 21, 2024, 8:07 p.m. UTC | #1
On 8/21/24 1:52 PM, Arsen Arsenović wrote:
> We now point out why a function is a coroutine, and where (the first
> return) is in the function.

OK.

> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (struct coroutine_info): Rename
> 	first_coro_keyword -> first_coro_expr.  The former name is no
> 	longer accurate.
> 	(coro_promise_type_found_p): Adjust accordingly.
> 	(coro_function_valid_p): Change how we diagnose 'return'
> 	statements in coroutines to also point out where a function was
> 	made a coroutine, and where 'return' was used.
> 	(find_coro_traits_template_decl): Rename kw parameter into loc,
> 	since it might not refer to a keyword always.
> 	(instantiate_coro_traits): Ditto.
> 	(find_coro_handle_template_decl): Ditto.
> 	(get_handle_type_address): Ditto.
> 	(get_handle_type_from_address): Ditto.
> 	(instantiate_coro_handle_for_promise_type): Ditto.
> 	(build_template_co_await_expr): Ditto.
> 	(finish_co_await_expr): Ditto.
> 	(finish_co_yield_expr): Ditto.
> 	(finish_co_return_stmt): Ditto.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to
> 	match new diagnostic.  Test more keyword combinations.
> ---
>   gcc/cp/coroutines.cc                          | 127 ++++++++++--------
>   .../co-return-syntax-08-bad-return.C          |  52 ++++++-
>   2 files changed, 119 insertions(+), 60 deletions(-)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index f7791cbfb9a6..81096784b4d7 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -93,8 +93,8 @@ struct GTY((for_user)) coroutine_info
>     tree promise_proxy; /* Likewise, a proxy promise instance.  */
>     tree from_address;  /* handle_type from_address function.  */
>     tree return_void;   /* The expression for p.return_void() if it exists.  */
> -  location_t first_coro_keyword; /* The location of the keyword that made this
> -				    function into a coroutine.  */
> +  location_t first_coro_expr; /* The location of the expression that turned
> +				 this funtion into a coroutine.  */
>     /* Flags to avoid repeated errors for per-function issues.  */
>     bool coro_ret_type_error_emitted;
>     bool coro_promise_error_emitted;
> @@ -285,7 +285,7 @@ static GTY(()) tree void_coro_handle_address;
>      Lookup the coroutine_traits template decl.  */
>   
>   static tree
> -find_coro_traits_template_decl (location_t kw)
> +find_coro_traits_template_decl (location_t loc)
>   {
>     /* If we are missing fundamental information, such as the traits, (or the
>        declaration found is not a type template), then don't emit an error for
> @@ -300,7 +300,7 @@ find_coro_traits_template_decl (location_t kw)
>       {
>         if (!traits_error_emitted)
>   	{
> -	  gcc_rich_location richloc (kw);
> +	  gcc_rich_location richloc (loc);
>   	  error_at (&richloc, "coroutines require a traits template; cannot"
>   		    " find %<%E::%E%>", std_node, coro_traits_identifier);
>   	  inform (&richloc, "perhaps %<#include <coroutine>%> is missing");
> @@ -315,7 +315,7 @@ find_coro_traits_template_decl (location_t kw)
>   /*  Instantiate Coroutine traits for the function signature.  */
>   
>   static tree
> -instantiate_coro_traits (tree fndecl, location_t kw)
> +instantiate_coro_traits (tree fndecl, location_t loc)
>   {
>     /* [coroutine.traits.primary]
>        So now build up a type list for the template <typename _R, typename...>.
> @@ -358,7 +358,7 @@ instantiate_coro_traits (tree fndecl, location_t kw)
>   
>     if (traits_class == error_mark_node)
>       {
> -      error_at (kw, "cannot instantiate %<coroutine traits%>");
> +      error_at (loc, "cannot instantiate %<coroutine traits%>");
>         return NULL_TREE;
>       }
>   
> @@ -368,7 +368,7 @@ instantiate_coro_traits (tree fndecl, location_t kw)
>   /* [coroutine.handle] */
>   
>   static tree
> -find_coro_handle_template_decl (location_t kw)
> +find_coro_handle_template_decl (location_t loc)
>   {
>     /* As for the coroutine traits, this error is per TU, so only emit
>       it once.  */
> @@ -380,7 +380,7 @@ find_coro_handle_template_decl (location_t kw)
>         || !DECL_CLASS_TEMPLATE_P (handle_decl))
>       {
>         if (!coro_handle_error_emitted)
> -	error_at (kw, "coroutines require a handle class template;"
> +	error_at (loc, "coroutines require a handle class template;"
>   		  " cannot find %<%E::%E%>", std_node, coro_handle_identifier);
>         coro_handle_error_emitted = true;
>         return NULL_TREE;
> @@ -394,21 +394,21 @@ find_coro_handle_template_decl (location_t kw)
>      void*.  If that is not the case, signals an error and returns NULL_TREE.  */
>   
>   static tree
> -get_handle_type_address (location_t kw, tree handle_type)
> +get_handle_type_address (location_t loc, tree handle_type)
>   {
>     tree addr_getter = lookup_member (handle_type, coro_address_identifier, 1,
>   				    0, tf_warning_or_error);
>     if (!addr_getter || addr_getter == error_mark_node)
>       {
>         qualified_name_lookup_error (handle_type, coro_address_identifier,
> -				   error_mark_node, kw);
> +				   error_mark_node, loc);
>         return NULL_TREE;
>       }
>   
>     if (!BASELINK_P (addr_getter)
>         || TREE_CODE (TREE_TYPE (addr_getter)) != METHOD_TYPE)
>       {
> -      error_at (kw, "%qE must be a non-overloaded method", addr_getter);
> +      error_at (loc, "%qE must be a non-overloaded method", addr_getter);
>         return NULL_TREE;
>       }
>   
> @@ -421,14 +421,14 @@ get_handle_type_address (location_t kw, tree handle_type)
>     /* Check that from_addr has the argument list ().  */
>     if (arg != void_list_node)
>       {
> -      error_at (kw, "%qE must take no arguments", addr_getter);
> +      error_at (loc, "%qE must take no arguments", addr_getter);
>         return NULL_TREE;
>       }
>   
>     tree ret_t = TREE_TYPE (fn_t);
>     if (!same_type_p (ret_t, ptr_type_node))
>       {
> -      error_at (kw, "%qE must return %qT, not %qT",
> +      error_at (loc, "%qE must return %qT, not %qT",
>   		addr_getter, ptr_type_node, ret_t);
>         return NULL_TREE;
>       }
> @@ -442,20 +442,20 @@ get_handle_type_address (location_t kw, tree handle_type)
>      NULL_TREE.  */
>   
>   static tree
> -get_handle_type_from_address (location_t kw, tree handle_type)
> +get_handle_type_from_address (location_t loc, tree handle_type)
>   {
>     tree from_addr = lookup_member (handle_type, coro_from_address_identifier, 1,
>   				  0, tf_warning_or_error);
>     if (!from_addr || from_addr == error_mark_node)
>       {
>         qualified_name_lookup_error (handle_type, coro_from_address_identifier,
> -				   error_mark_node, kw);
> +				   error_mark_node, loc);
>         return NULL_TREE;
>       }
>     if (!BASELINK_P (from_addr)
>         || TREE_CODE (TREE_TYPE (from_addr)) != FUNCTION_TYPE)
>       {
> -      error_at (kw, "%qE must be a non-overloaded static function", from_addr);
> +      error_at (loc, "%qE must be a non-overloaded static function", from_addr);
>         return NULL_TREE;
>       }
>   
> @@ -466,14 +466,14 @@ get_handle_type_from_address (location_t kw, tree handle_type)
>         || !same_type_p (TREE_VALUE (arg), ptr_type_node)
>         || TREE_CHAIN (arg) != void_list_node)
>       {
> -      error_at (kw, "%qE must take a single %qT", from_addr, ptr_type_node);
> +      error_at (loc, "%qE must take a single %qT", from_addr, ptr_type_node);
>         return NULL_TREE;
>       }
>   
>     tree ret_t = TREE_TYPE (fn_t);
>     if (!same_type_p (ret_t, handle_type))
>       {
> -      error_at (kw, "%qE must return %qT, not %qT",
> +      error_at (loc, "%qE must return %qT, not %qT",
>   		from_addr, handle_type, ret_t);
>         return NULL_TREE;
>       }
> @@ -482,7 +482,7 @@ get_handle_type_from_address (location_t kw, tree handle_type)
>   }
>   
>   static tree
> -instantiate_coro_handle_for_promise_type (location_t kw, tree promise_type)
> +instantiate_coro_handle_for_promise_type (location_t loc, tree promise_type)
>   {
>     /* So now build up a type list for the template, one entry, the promise.  */
>     tree targ = make_tree_vec (1);
> @@ -495,7 +495,7 @@ instantiate_coro_handle_for_promise_type (location_t kw, tree promise_type)
>   
>     if (handle_type == error_mark_node)
>       {
> -      error_at (kw, "cannot instantiate a %<coroutine handle%> for"
> +      error_at (loc, "cannot instantiate a %<coroutine handle%> for"
>   		" promise type %qT", promise_type);
>         return NULL_TREE;
>       }
> @@ -674,8 +674,8 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
>   	= build_lang_decl (VAR_DECL, coro_promise_id,
>   			   coro_info->promise_type);
>   
> -      /* Note where we first saw a coroutine keyword.  */
> -      coro_info->first_coro_keyword = loc;
> +      /* Note where we were when we made this function into a coroutine.  */
> +      coro_info->first_coro_expr = loc;
>       }
>   
>     return true;
> @@ -968,11 +968,24 @@ 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%>?");
> +      auto coro_info = get_coroutine_info (fndecl);
> +      gcc_checking_assert (coro_info->first_coro_expr);
> +
> +      walk_tree_fn retf = [] (tree* pt, int*, void*)
> +      {
> +	if (*pt && TREE_CODE (*pt) == RETURN_EXPR)
> +	  return *pt;
> +	return NULL_TREE;
> +      };
> +
> +      tree ret = cp_walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl),
> +						  retf, nullptr);
> +      auto ret_loc = EXPR_LOCATION (ret);
> +      auto_diagnostic_group diaggrp;
> +      error_at (ret_loc, "a %<return%> statement is not allowed in coroutine;"
> +		" did you mean %<co_return%>?");
> +      inform (coro_info->first_coro_expr,
> +	      "function was made a coroutine here");
>         return false;
>       }
>   
> @@ -1051,9 +1064,9 @@ coro_diagnose_throwing_final_aw_expr (tree expr)
>   /* Build a co_await suitable for later expansion.  */
>   
>   tree
> -build_template_co_await_expr (location_t kw, tree type, tree expr, tree kind)
> +build_template_co_await_expr (location_t loc, tree type, tree expr, tree kind)
>   {
> -  tree aw_expr = build5_loc (kw, CO_AWAIT_EXPR, type, expr,
> +  tree aw_expr = build5_loc (loc, CO_AWAIT_EXPR, type, expr,
>   			     NULL_TREE, NULL_TREE, NULL_TREE,
>   			     kind);
>     TREE_SIDE_EFFECTS (aw_expr) = true;
> @@ -1326,12 +1339,12 @@ coro_dependent_p (tree expr, tree traits_class)
>   }
>   
>   tree
> -finish_co_await_expr (location_t kw, tree expr)
> +finish_co_await_expr (location_t loc, tree expr)
>   {
>     if (!expr || error_operand_p (expr))
>       return error_mark_node;
>   
> -  if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
> +  if (!coro_common_keyword_context_valid_p (current_function_decl, loc,
>   					    "co_await"))
>       return error_mark_node;
>   
> @@ -1345,11 +1358,11 @@ finish_co_await_expr (location_t kw, tree expr)
>     suppress_warning (current_function_decl, OPT_Wreturn_type);
>   
>     /* Prepare for coroutine transformations.  */
> -  if (!ensure_coro_initialized (kw))
> +  if (!ensure_coro_initialized (loc))
>       return error_mark_node;
>   
>     /* Get our traits class.  */
> -  tree traits_class = coro_get_traits_class (current_function_decl, kw);
> +  tree traits_class = coro_get_traits_class (current_function_decl, loc);
>   
>     /* Defer expansion when we are processing a template, unless the traits type
>        and expression would not be dependent.  In the case that the types are
> @@ -1360,12 +1373,12 @@ finish_co_await_expr (location_t kw, tree expr)
>        and just return early with a NULL_TREE type (indicating that we cannot
>        compute the type yet).  */
>     if (coro_dependent_p (expr, traits_class))
> -    return build_template_co_await_expr (kw, NULL_TREE, expr,
> +    return build_template_co_await_expr (loc, NULL_TREE, expr,
>   					 integer_zero_node);
>   
>     /* We must be able to look up the "await_transform" method in the scope of
>        the promise type, and obtain its return type.  */
> -  if (!coro_promise_type_found_p (current_function_decl, kw))
> +  if (!coro_promise_type_found_p (current_function_decl, loc))
>       return error_mark_node;
>   
>     /* [expr.await] 3.2
> @@ -1373,7 +1386,7 @@ finish_co_await_expr (location_t kw, tree expr)
>        'await_transform()'.  */
>     tree at_meth
>       = lookup_promise_method (current_function_decl,
> -			     coro_await_transform_identifier, kw,
> +			     coro_await_transform_identifier, loc,
>   			     /*musthave=*/false);
>     if (at_meth == error_mark_node)
>       return error_mark_node;
> @@ -1398,7 +1411,7 @@ finish_co_await_expr (location_t kw, tree expr)
>       }
>   
>     /* Now we want to build co_await a.  */
> -  return build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT, expr);
> +  return build_co_await (loc, a, CO_AWAIT_SUSPEND_POINT, expr);
>   }
>   
>   /* Take the EXPR given and attempt to build:
> @@ -1406,13 +1419,13 @@ finish_co_await_expr (location_t kw, tree expr)
>      per [expr.yield] para 1. */
>   
>   tree
> -finish_co_yield_expr (location_t kw, tree expr)
> +finish_co_yield_expr (location_t loc, tree expr)
>   {
>     if (!expr || error_operand_p (expr))
>       return error_mark_node;
>   
>     /* Check the general requirements and simple syntax errors.  */
> -  if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
> +  if (!coro_common_keyword_context_valid_p (current_function_decl, loc,
>   					    "co_yield"))
>       return error_mark_node;
>   
> @@ -1426,11 +1439,11 @@ finish_co_yield_expr (location_t kw, tree expr)
>     suppress_warning (current_function_decl, OPT_Wreturn_type);
>   
>     /* Prepare for coroutine transformations.  */
> -  if (!ensure_coro_initialized (kw))
> +  if (!ensure_coro_initialized (loc))
>       return error_mark_node;
>   
>     /* Get our traits class.  */
> -  tree traits_class = coro_get_traits_class (current_function_decl, kw);
> +  tree traits_class = coro_get_traits_class (current_function_decl, loc);
>   
>     /* Defer expansion when we are processing a template; see note in
>        finish_co_await_expr.  Also note that a yield is equivalent to
> @@ -1440,9 +1453,9 @@ finish_co_yield_expr (location_t kw, tree expr)
>         If either p or EXPR are type-dependent, then the whole expression is
>         certainly type-dependent, and we can't proceed.  */
>     if (coro_dependent_p (expr, traits_class))
> -    return build2_loc (kw, CO_YIELD_EXPR, NULL_TREE, expr, NULL_TREE);
> +    return build2_loc (loc, CO_YIELD_EXPR, NULL_TREE, expr, NULL_TREE);
>   
> -  if (!coro_promise_type_found_p (current_function_decl, kw))
> +  if (!coro_promise_type_found_p (current_function_decl, loc))
>       /* We must be able to look up the "yield_value" method in the scope of
>          the promise type, and obtain its return type.  */
>       return error_mark_node;
> @@ -1455,7 +1468,7 @@ finish_co_yield_expr (location_t kw, tree expr)
>     vec<tree, va_gc> *args = make_tree_vector_single (expr);
>     tree yield_call
>       = coro_build_promise_expression (current_function_decl, NULL,
> -				     coro_yield_value_identifier, kw,
> +				     coro_yield_value_identifier, loc,
>   				     &args, /*musthave=*/true);
>     release_tree_vector (args);
>   
> @@ -1463,7 +1476,7 @@ finish_co_yield_expr (location_t kw, tree expr)
>        Noting that for co_yield, there is no evaluation of any potential
>        promise transform_await(), so we call build_co_await directly.  */
>   
> -  tree op = build_co_await (kw, yield_call, CO_YIELD_SUSPEND_POINT);
> +  tree op = build_co_await (loc, yield_call, CO_YIELD_SUSPEND_POINT);
>     if (op != error_mark_node)
>       {
>         if (REFERENCE_REF_P (op))
> @@ -1474,11 +1487,11 @@ finish_co_yield_expr (location_t kw, tree expr)
>         if (TREE_CODE (op) == TARGET_EXPR)
>   	{
>   	  tree t = TREE_OPERAND (op, 1);
> -	  t = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (t), expr, t);
> +	  t = build2_loc (loc, CO_YIELD_EXPR, TREE_TYPE (t), expr, t);
>   	  TREE_OPERAND (op, 1) = t;
>   	}
>         else
> -	op = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
> +	op = build2_loc (loc, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
>         TREE_SIDE_EFFECTS (op) = 1;
>         op = convert_from_reference (op);
>       }
> @@ -1493,7 +1506,7 @@ finish_co_yield_expr (location_t kw, tree expr)
>      in with the actual frame version when the function is transformed.  */
>   
>   tree
> -finish_co_return_stmt (location_t kw, tree expr)
> +finish_co_return_stmt (location_t loc, tree expr)
>   {
>     if (expr)
>       STRIP_ANY_LOCATION_WRAPPER (expr);
> @@ -1503,7 +1516,7 @@ finish_co_return_stmt (location_t kw, tree expr)
>   
>     /* If it fails the following test, the function is not permitted to be a
>        coroutine, so the co_return statement is erroneous.  */
> -  if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
> +  if (!coro_common_keyword_context_valid_p (current_function_decl, loc,
>   					    "co_return"))
>       return error_mark_node;
>   
> @@ -1518,11 +1531,11 @@ finish_co_return_stmt (location_t kw, tree expr)
>     suppress_warning (current_function_decl, OPT_Wreturn_type);
>   
>     /* Prepare for coroutine transformations.  */
> -  if (!ensure_coro_initialized (kw))
> +  if (!ensure_coro_initialized (loc))
>       return error_mark_node;
>   
>     /* Get our traits class.  */
> -  tree traits_class = coro_get_traits_class (current_function_decl, kw);
> +  tree traits_class = coro_get_traits_class (current_function_decl, loc);
>   
>     if (processing_template_decl
>         && check_for_bare_parameter_packs (expr))
> @@ -1534,13 +1547,13 @@ finish_co_return_stmt (location_t kw, tree expr)
>       {
>         /* co_return expressions are always void type, regardless of the
>   	 expression type.  */
> -      expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node,
> +      expr = build2_loc (loc, CO_RETURN_EXPR, void_type_node,
>   			 expr, NULL_TREE);
>         expr = maybe_cleanup_point_expr_void (expr);
>         return add_stmt (expr);
>       }
>   
> -  if (!coro_promise_type_found_p (current_function_decl, kw))
> +  if (!coro_promise_type_found_p (current_function_decl, loc))
>       return error_mark_node;
>   
>     /* Suppress -Wreturn-type for co_return, we need to check indirectly
> @@ -1565,7 +1578,7 @@ finish_co_return_stmt (location_t kw, tree expr)
>     tree co_ret_call = error_mark_node;
>     if (expr == NULL_TREE || VOID_TYPE_P (TREE_TYPE (expr)))
>       co_ret_call
> -      = get_coroutine_return_void_expr (current_function_decl, kw, true);
> +      = get_coroutine_return_void_expr (current_function_decl, loc, true);
>     else
>       {
>         /* [class.copy.elision] / 3.
> @@ -1583,17 +1596,17 @@ finish_co_return_stmt (location_t kw, tree expr)
>         releasing_vec args = make_tree_vector_single (arg);
>         co_ret_call
>   	= coro_build_promise_expression (current_function_decl, NULL,
> -					 coro_return_value_identifier, kw,
> +					 coro_return_value_identifier, loc,
>   					 &args, /*musthave=*/true);
>       }
>   
>     /* Makes no sense for a co-routine really. */
>     if (TREE_THIS_VOLATILE (current_function_decl))
> -    warning_at (kw, 0,
> +    warning_at (loc, 0,
>   		"function declared %<noreturn%> has a"
>   		" %<co_return%> statement");
>   
> -  expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
> +  expr = build2_loc (loc, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
>     expr = maybe_cleanup_point_expr_void (expr);
>     return add_stmt (expr);
>   }
> 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} }
>   }
diff mbox series

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index f7791cbfb9a6..81096784b4d7 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -93,8 +93,8 @@  struct GTY((for_user)) coroutine_info
   tree promise_proxy; /* Likewise, a proxy promise instance.  */
   tree from_address;  /* handle_type from_address function.  */
   tree return_void;   /* The expression for p.return_void() if it exists.  */
-  location_t first_coro_keyword; /* The location of the keyword that made this
-				    function into a coroutine.  */
+  location_t first_coro_expr; /* The location of the expression that turned
+				 this funtion into a coroutine.  */
   /* Flags to avoid repeated errors for per-function issues.  */
   bool coro_ret_type_error_emitted;
   bool coro_promise_error_emitted;
@@ -285,7 +285,7 @@  static GTY(()) tree void_coro_handle_address;
    Lookup the coroutine_traits template decl.  */
 
 static tree
-find_coro_traits_template_decl (location_t kw)
+find_coro_traits_template_decl (location_t loc)
 {
   /* If we are missing fundamental information, such as the traits, (or the
      declaration found is not a type template), then don't emit an error for
@@ -300,7 +300,7 @@  find_coro_traits_template_decl (location_t kw)
     {
       if (!traits_error_emitted)
 	{
-	  gcc_rich_location richloc (kw);
+	  gcc_rich_location richloc (loc);
 	  error_at (&richloc, "coroutines require a traits template; cannot"
 		    " find %<%E::%E%>", std_node, coro_traits_identifier);
 	  inform (&richloc, "perhaps %<#include <coroutine>%> is missing");
@@ -315,7 +315,7 @@  find_coro_traits_template_decl (location_t kw)
 /*  Instantiate Coroutine traits for the function signature.  */
 
 static tree
-instantiate_coro_traits (tree fndecl, location_t kw)
+instantiate_coro_traits (tree fndecl, location_t loc)
 {
   /* [coroutine.traits.primary]
      So now build up a type list for the template <typename _R, typename...>.
@@ -358,7 +358,7 @@  instantiate_coro_traits (tree fndecl, location_t kw)
 
   if (traits_class == error_mark_node)
     {
-      error_at (kw, "cannot instantiate %<coroutine traits%>");
+      error_at (loc, "cannot instantiate %<coroutine traits%>");
       return NULL_TREE;
     }
 
@@ -368,7 +368,7 @@  instantiate_coro_traits (tree fndecl, location_t kw)
 /* [coroutine.handle] */
 
 static tree
-find_coro_handle_template_decl (location_t kw)
+find_coro_handle_template_decl (location_t loc)
 {
   /* As for the coroutine traits, this error is per TU, so only emit
     it once.  */
@@ -380,7 +380,7 @@  find_coro_handle_template_decl (location_t kw)
       || !DECL_CLASS_TEMPLATE_P (handle_decl))
     {
       if (!coro_handle_error_emitted)
-	error_at (kw, "coroutines require a handle class template;"
+	error_at (loc, "coroutines require a handle class template;"
 		  " cannot find %<%E::%E%>", std_node, coro_handle_identifier);
       coro_handle_error_emitted = true;
       return NULL_TREE;
@@ -394,21 +394,21 @@  find_coro_handle_template_decl (location_t kw)
    void*.  If that is not the case, signals an error and returns NULL_TREE.  */
 
 static tree
-get_handle_type_address (location_t kw, tree handle_type)
+get_handle_type_address (location_t loc, tree handle_type)
 {
   tree addr_getter = lookup_member (handle_type, coro_address_identifier, 1,
 				    0, tf_warning_or_error);
   if (!addr_getter || addr_getter == error_mark_node)
     {
       qualified_name_lookup_error (handle_type, coro_address_identifier,
-				   error_mark_node, kw);
+				   error_mark_node, loc);
       return NULL_TREE;
     }
 
   if (!BASELINK_P (addr_getter)
       || TREE_CODE (TREE_TYPE (addr_getter)) != METHOD_TYPE)
     {
-      error_at (kw, "%qE must be a non-overloaded method", addr_getter);
+      error_at (loc, "%qE must be a non-overloaded method", addr_getter);
       return NULL_TREE;
     }
 
@@ -421,14 +421,14 @@  get_handle_type_address (location_t kw, tree handle_type)
   /* Check that from_addr has the argument list ().  */
   if (arg != void_list_node)
     {
-      error_at (kw, "%qE must take no arguments", addr_getter);
+      error_at (loc, "%qE must take no arguments", addr_getter);
       return NULL_TREE;
     }
 
   tree ret_t = TREE_TYPE (fn_t);
   if (!same_type_p (ret_t, ptr_type_node))
     {
-      error_at (kw, "%qE must return %qT, not %qT",
+      error_at (loc, "%qE must return %qT, not %qT",
 		addr_getter, ptr_type_node, ret_t);
       return NULL_TREE;
     }
@@ -442,20 +442,20 @@  get_handle_type_address (location_t kw, tree handle_type)
    NULL_TREE.  */
 
 static tree
-get_handle_type_from_address (location_t kw, tree handle_type)
+get_handle_type_from_address (location_t loc, tree handle_type)
 {
   tree from_addr = lookup_member (handle_type, coro_from_address_identifier, 1,
 				  0, tf_warning_or_error);
   if (!from_addr || from_addr == error_mark_node)
     {
       qualified_name_lookup_error (handle_type, coro_from_address_identifier,
-				   error_mark_node, kw);
+				   error_mark_node, loc);
       return NULL_TREE;
     }
   if (!BASELINK_P (from_addr)
       || TREE_CODE (TREE_TYPE (from_addr)) != FUNCTION_TYPE)
     {
-      error_at (kw, "%qE must be a non-overloaded static function", from_addr);
+      error_at (loc, "%qE must be a non-overloaded static function", from_addr);
       return NULL_TREE;
     }
 
@@ -466,14 +466,14 @@  get_handle_type_from_address (location_t kw, tree handle_type)
       || !same_type_p (TREE_VALUE (arg), ptr_type_node)
       || TREE_CHAIN (arg) != void_list_node)
     {
-      error_at (kw, "%qE must take a single %qT", from_addr, ptr_type_node);
+      error_at (loc, "%qE must take a single %qT", from_addr, ptr_type_node);
       return NULL_TREE;
     }
 
   tree ret_t = TREE_TYPE (fn_t);
   if (!same_type_p (ret_t, handle_type))
     {
-      error_at (kw, "%qE must return %qT, not %qT",
+      error_at (loc, "%qE must return %qT, not %qT",
 		from_addr, handle_type, ret_t);
       return NULL_TREE;
     }
@@ -482,7 +482,7 @@  get_handle_type_from_address (location_t kw, tree handle_type)
 }
 
 static tree
-instantiate_coro_handle_for_promise_type (location_t kw, tree promise_type)
+instantiate_coro_handle_for_promise_type (location_t loc, tree promise_type)
 {
   /* So now build up a type list for the template, one entry, the promise.  */
   tree targ = make_tree_vec (1);
@@ -495,7 +495,7 @@  instantiate_coro_handle_for_promise_type (location_t kw, tree promise_type)
 
   if (handle_type == error_mark_node)
     {
-      error_at (kw, "cannot instantiate a %<coroutine handle%> for"
+      error_at (loc, "cannot instantiate a %<coroutine handle%> for"
 		" promise type %qT", promise_type);
       return NULL_TREE;
     }
@@ -674,8 +674,8 @@  coro_promise_type_found_p (tree fndecl, location_t loc)
 	= build_lang_decl (VAR_DECL, coro_promise_id,
 			   coro_info->promise_type);
 
-      /* Note where we first saw a coroutine keyword.  */
-      coro_info->first_coro_keyword = loc;
+      /* Note where we were when we made this function into a coroutine.  */
+      coro_info->first_coro_expr = loc;
     }
 
   return true;
@@ -968,11 +968,24 @@  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%>?");
+      auto coro_info = get_coroutine_info (fndecl);
+      gcc_checking_assert (coro_info->first_coro_expr);
+
+      walk_tree_fn retf = [] (tree* pt, int*, void*)
+      {
+	if (*pt && TREE_CODE (*pt) == RETURN_EXPR)
+	  return *pt;
+	return NULL_TREE;
+      };
+
+      tree ret = cp_walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl),
+						  retf, nullptr);
+      auto ret_loc = EXPR_LOCATION (ret);
+      auto_diagnostic_group diaggrp;
+      error_at (ret_loc, "a %<return%> statement is not allowed in coroutine;"
+		" did you mean %<co_return%>?");
+      inform (coro_info->first_coro_expr,
+	      "function was made a coroutine here");
       return false;
     }
 
@@ -1051,9 +1064,9 @@  coro_diagnose_throwing_final_aw_expr (tree expr)
 /* Build a co_await suitable for later expansion.  */
 
 tree
-build_template_co_await_expr (location_t kw, tree type, tree expr, tree kind)
+build_template_co_await_expr (location_t loc, tree type, tree expr, tree kind)
 {
-  tree aw_expr = build5_loc (kw, CO_AWAIT_EXPR, type, expr,
+  tree aw_expr = build5_loc (loc, CO_AWAIT_EXPR, type, expr,
 			     NULL_TREE, NULL_TREE, NULL_TREE,
 			     kind);
   TREE_SIDE_EFFECTS (aw_expr) = true;
@@ -1326,12 +1339,12 @@  coro_dependent_p (tree expr, tree traits_class)
 }
 
 tree
-finish_co_await_expr (location_t kw, tree expr)
+finish_co_await_expr (location_t loc, tree expr)
 {
   if (!expr || error_operand_p (expr))
     return error_mark_node;
 
-  if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
+  if (!coro_common_keyword_context_valid_p (current_function_decl, loc,
 					    "co_await"))
     return error_mark_node;
 
@@ -1345,11 +1358,11 @@  finish_co_await_expr (location_t kw, tree expr)
   suppress_warning (current_function_decl, OPT_Wreturn_type);
 
   /* Prepare for coroutine transformations.  */
-  if (!ensure_coro_initialized (kw))
+  if (!ensure_coro_initialized (loc))
     return error_mark_node;
 
   /* Get our traits class.  */
-  tree traits_class = coro_get_traits_class (current_function_decl, kw);
+  tree traits_class = coro_get_traits_class (current_function_decl, loc);
 
   /* Defer expansion when we are processing a template, unless the traits type
      and expression would not be dependent.  In the case that the types are
@@ -1360,12 +1373,12 @@  finish_co_await_expr (location_t kw, tree expr)
      and just return early with a NULL_TREE type (indicating that we cannot
      compute the type yet).  */
   if (coro_dependent_p (expr, traits_class))
-    return build_template_co_await_expr (kw, NULL_TREE, expr,
+    return build_template_co_await_expr (loc, NULL_TREE, expr,
 					 integer_zero_node);
 
   /* We must be able to look up the "await_transform" method in the scope of
      the promise type, and obtain its return type.  */
-  if (!coro_promise_type_found_p (current_function_decl, kw))
+  if (!coro_promise_type_found_p (current_function_decl, loc))
     return error_mark_node;
 
   /* [expr.await] 3.2
@@ -1373,7 +1386,7 @@  finish_co_await_expr (location_t kw, tree expr)
      'await_transform()'.  */
   tree at_meth
     = lookup_promise_method (current_function_decl,
-			     coro_await_transform_identifier, kw,
+			     coro_await_transform_identifier, loc,
 			     /*musthave=*/false);
   if (at_meth == error_mark_node)
     return error_mark_node;
@@ -1398,7 +1411,7 @@  finish_co_await_expr (location_t kw, tree expr)
     }
 
   /* Now we want to build co_await a.  */
-  return build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT, expr);
+  return build_co_await (loc, a, CO_AWAIT_SUSPEND_POINT, expr);
 }
 
 /* Take the EXPR given and attempt to build:
@@ -1406,13 +1419,13 @@  finish_co_await_expr (location_t kw, tree expr)
    per [expr.yield] para 1. */
 
 tree
-finish_co_yield_expr (location_t kw, tree expr)
+finish_co_yield_expr (location_t loc, tree expr)
 {
   if (!expr || error_operand_p (expr))
     return error_mark_node;
 
   /* Check the general requirements and simple syntax errors.  */
-  if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
+  if (!coro_common_keyword_context_valid_p (current_function_decl, loc,
 					    "co_yield"))
     return error_mark_node;
 
@@ -1426,11 +1439,11 @@  finish_co_yield_expr (location_t kw, tree expr)
   suppress_warning (current_function_decl, OPT_Wreturn_type);
 
   /* Prepare for coroutine transformations.  */
-  if (!ensure_coro_initialized (kw))
+  if (!ensure_coro_initialized (loc))
     return error_mark_node;
 
   /* Get our traits class.  */
-  tree traits_class = coro_get_traits_class (current_function_decl, kw);
+  tree traits_class = coro_get_traits_class (current_function_decl, loc);
 
   /* Defer expansion when we are processing a template; see note in
      finish_co_await_expr.  Also note that a yield is equivalent to
@@ -1440,9 +1453,9 @@  finish_co_yield_expr (location_t kw, tree expr)
       If either p or EXPR are type-dependent, then the whole expression is
       certainly type-dependent, and we can't proceed.  */
   if (coro_dependent_p (expr, traits_class))
-    return build2_loc (kw, CO_YIELD_EXPR, NULL_TREE, expr, NULL_TREE);
+    return build2_loc (loc, CO_YIELD_EXPR, NULL_TREE, expr, NULL_TREE);
 
-  if (!coro_promise_type_found_p (current_function_decl, kw))
+  if (!coro_promise_type_found_p (current_function_decl, loc))
     /* We must be able to look up the "yield_value" method in the scope of
        the promise type, and obtain its return type.  */
     return error_mark_node;
@@ -1455,7 +1468,7 @@  finish_co_yield_expr (location_t kw, tree expr)
   vec<tree, va_gc> *args = make_tree_vector_single (expr);
   tree yield_call
     = coro_build_promise_expression (current_function_decl, NULL,
-				     coro_yield_value_identifier, kw,
+				     coro_yield_value_identifier, loc,
 				     &args, /*musthave=*/true);
   release_tree_vector (args);
 
@@ -1463,7 +1476,7 @@  finish_co_yield_expr (location_t kw, tree expr)
      Noting that for co_yield, there is no evaluation of any potential
      promise transform_await(), so we call build_co_await directly.  */
 
-  tree op = build_co_await (kw, yield_call, CO_YIELD_SUSPEND_POINT);
+  tree op = build_co_await (loc, yield_call, CO_YIELD_SUSPEND_POINT);
   if (op != error_mark_node)
     {
       if (REFERENCE_REF_P (op))
@@ -1474,11 +1487,11 @@  finish_co_yield_expr (location_t kw, tree expr)
       if (TREE_CODE (op) == TARGET_EXPR)
 	{
 	  tree t = TREE_OPERAND (op, 1);
-	  t = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (t), expr, t);
+	  t = build2_loc (loc, CO_YIELD_EXPR, TREE_TYPE (t), expr, t);
 	  TREE_OPERAND (op, 1) = t;
 	}
       else
-	op = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
+	op = build2_loc (loc, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
       TREE_SIDE_EFFECTS (op) = 1;
       op = convert_from_reference (op);
     }
@@ -1493,7 +1506,7 @@  finish_co_yield_expr (location_t kw, tree expr)
    in with the actual frame version when the function is transformed.  */
 
 tree
-finish_co_return_stmt (location_t kw, tree expr)
+finish_co_return_stmt (location_t loc, tree expr)
 {
   if (expr)
     STRIP_ANY_LOCATION_WRAPPER (expr);
@@ -1503,7 +1516,7 @@  finish_co_return_stmt (location_t kw, tree expr)
 
   /* If it fails the following test, the function is not permitted to be a
      coroutine, so the co_return statement is erroneous.  */
-  if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
+  if (!coro_common_keyword_context_valid_p (current_function_decl, loc,
 					    "co_return"))
     return error_mark_node;
 
@@ -1518,11 +1531,11 @@  finish_co_return_stmt (location_t kw, tree expr)
   suppress_warning (current_function_decl, OPT_Wreturn_type);
 
   /* Prepare for coroutine transformations.  */
-  if (!ensure_coro_initialized (kw))
+  if (!ensure_coro_initialized (loc))
     return error_mark_node;
 
   /* Get our traits class.  */
-  tree traits_class = coro_get_traits_class (current_function_decl, kw);
+  tree traits_class = coro_get_traits_class (current_function_decl, loc);
 
   if (processing_template_decl
       && check_for_bare_parameter_packs (expr))
@@ -1534,13 +1547,13 @@  finish_co_return_stmt (location_t kw, tree expr)
     {
       /* co_return expressions are always void type, regardless of the
 	 expression type.  */
-      expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node,
+      expr = build2_loc (loc, CO_RETURN_EXPR, void_type_node,
 			 expr, NULL_TREE);
       expr = maybe_cleanup_point_expr_void (expr);
       return add_stmt (expr);
     }
 
-  if (!coro_promise_type_found_p (current_function_decl, kw))
+  if (!coro_promise_type_found_p (current_function_decl, loc))
     return error_mark_node;
 
   /* Suppress -Wreturn-type for co_return, we need to check indirectly
@@ -1565,7 +1578,7 @@  finish_co_return_stmt (location_t kw, tree expr)
   tree co_ret_call = error_mark_node;
   if (expr == NULL_TREE || VOID_TYPE_P (TREE_TYPE (expr)))
     co_ret_call
-      = get_coroutine_return_void_expr (current_function_decl, kw, true);
+      = get_coroutine_return_void_expr (current_function_decl, loc, true);
   else
     {
       /* [class.copy.elision] / 3.
@@ -1583,17 +1596,17 @@  finish_co_return_stmt (location_t kw, tree expr)
       releasing_vec args = make_tree_vector_single (arg);
       co_ret_call
 	= coro_build_promise_expression (current_function_decl, NULL,
-					 coro_return_value_identifier, kw,
+					 coro_return_value_identifier, loc,
 					 &args, /*musthave=*/true);
     }
 
   /* Makes no sense for a co-routine really. */
   if (TREE_THIS_VOLATILE (current_function_decl))
-    warning_at (kw, 0,
+    warning_at (loc, 0,
 		"function declared %<noreturn%> has a"
 		" %<co_return%> statement");
 
-  expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
+  expr = build2_loc (loc, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
   expr = maybe_cleanup_point_expr_void (expr);
   return add_stmt (expr);
 }
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} }
 }