Message ID | 20240807234811.36513-1-arsen@aarsen.me |
---|---|
State | New |
Headers | show |
Series | c++: improve diagnostic of 'return's in coroutines | expand |
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
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} } }