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} }
  }

Reply via email to