[clang-format] Add better support for co-routinues

[MyDeveloperDay]

Summary

Responding to a Discord call to help D113977: [Coroutine] Warn deprecated 'std::experimental::coro' uses and heavily inspired by the unlanded D34225: [clang-format] Teach clang-format how to handle C++ coroutines add some support to help coroutinues from not being formatted from

for co_await(auto elt : seq)

to

for
co_await(auto elt : seq)

Because of the dominance of clang-format in the C++ community, I don't think we should make it the blocker that prevents users from embracing the newer parts of the standard because we butcher the layout of some of the new constucts.

[ChuanqiXu]

clang/unittests/Format/FormatTest.cpp

22749

It may be worth to add following tests:

co_await co_await co_await foo();
co_await foo().bar();
co_await [this](int a, int b) -> Task {\n
    co_return co_await foo();\n
}(x, y);
co_await [this]() -> Task { co_return x; }

22762–22763

These two statements looks invalid.

22766

I think it would be better to add two tests:

co_return x;
co_return co_await foo();
co_return co_yield foo();

[MyDeveloperDay]

Add more tests

clang/unittests/Format/FormatTest.cpp

22762–22763

do you mean these two?

verifyFormat("int x = co_return foo();");
verifyFormat("int x = (co_return foo());");

I lifted these from D34225: [clang-format] Teach clang-format how to handle C++ coroutines

[ChuanqiXu]

clang/unittests/Format/FormatTest.cpp

22762–22763

yeah, since it is meaningless to me since we couldn't evaluate the value of co_return expression.

[• Quuxplusone]

LGTM % comments. I agree with all the formatting decisions shown in the new tests.

clang/docs/ReleaseNotes.rst

270

s/nues/nes/
In fact, you might just say - Improved support for C++20 Modules and Coroutines.

clang/lib/Format/TokenAnnotator.cpp

2957

The comment style on line 2953 seems strictly more useful to me. Is it correct to say

// co_await (x), co_yield (x), co_return (x)

and before line 2961,

// co_return;

FYI, co_await; and co_yield; are not currently valid C++; but I agree with the formatting you've chosen here. Please add a test for "co_await;" and "co_yield;" just to make sure it doesn't regress (even though it's invalid C++20).
Aside: I could even see co_yield; one day becoming valid C++ ("yielding void" to its awaiter).

clang/unittests/Format/FormatTest.cpp

22727

s/CoRoutine/Coroutine/g
Nits:

• I'd just call this function CoroutineForCoawait
• Why "for co_await (auto x : range())\n ;" as one string on line 22715, but then split strings on lines 22716-22718 and 22719-22721? Wouldn't it look simpler to use e.g. "for co_await (auto i : arr) {\n}" on line 22719?
• Line 22716 is not using co_await — is this deliberate or accidental?
• It might make sense to put all for-related tests close together in the test file — for, for co_await, for constexpr, template for, whatever else we come up with. I don't know if there are any tests for anything but vanilla for at the moment.

22741

22745

just to make sure the name int isn't being treated as magic by clang-format

22762–22763

I agree with @ChuanqiXu: int x = co_return foo(); is invalid and unrealistic syntax. As usual, I'm ambivalent on whether it makes sense to test invalid inputs (since we still don't want to crash on them).

22770

IIRC, before we lexed co_yield as a keyword, we used to do co_yield++ n;. I don't see any way for co_yield n++; to get misformatted, though: co_yieldn++; would obviously never happen.

[MyDeveloperDay]

Address review comments

clang/unittests/Format/FormatTest.cpp

22741

naming of the tests is to allow easy running of all CoRoutine tests

./FormatTests --gtest_filter=*CoRoutine*

22745

I'd like to add this, sometimes its nice to have tests which might traditionally challenge the code to behave reasonable even if the syntax is invalid

22770

let add both ways around to ensure its behaving.

[• Quuxplusone]

clang/unittests/Format/FormatTest.cpp

22741

That's a good rationale for consistency, but the English/C++ word is still "coroutine", not "CoRoutine." If there are other places that need changing, maybe it makes sense to mass-rename in a separate commit. The main thing I'm saying is "CoRoutine" is universally wrong. :)

(I also don't see why *Coroutine* should be a more useful dimension to want to filter on, than *Cpp20* or *Keywords* or *Unary* or whatever. I'd also be surprised if clang-format's tests ever run slow enough that filtering is desired. But I don't know and will defer to you-or-whoever.)

[ChuanqiXu]

LGTM. Thanks!

clang/docs/ReleaseNotes.rst

 - Option ``QualifierOrder`` has been added to allow the order
   `const` `volatile` `static` `inline` `constexpr` `restrict`
   to be controlled relative to the `type`.
 
 - Add a ``Custom`` style to ``SpaceBeforeParens``, to better configure the
   space before parentheses. The custom options can be set using
   ``SpaceBeforeParensOptions``.
 
-- Improved Cpp20 Modules support.
+- Improved C++20 Modules and Coroutines support.
 
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------
 
 - ...

clang/lib/Format/TokenAnnotator.cpp

case tok::kw_for:
         // x.for and {for: ...}
         if ((Tok->Previous && Tok->Previous->is(tok::period)) ||
             (Tok->Next && Tok->Next->is(tok::colon)))
           break;
         // JS' for await ( ...
         if (CurrentToken && CurrentToken->is(Keywords.kw_await))
           next();
       }
+      if (Style.isCpp() && CurrentToken && CurrentToken->is(tok::kw_co_await))
+        next();
       Contexts.back().ColonIsForRangeExpr = true;
       next();
       if (!parseParens())
         return false;
       break;
     case tok::l_paren:
       // When faced with 'operator()()', the kw_operator handler incorrectly
       // marks the first l_paren as a OverloadedOperatorLParen. Here, we make
if (Right.is(tok::r_paren) && Right.MatchingParen &&
         isKeywordWithCondition(*Right.MatchingParen->Previous))
       return true;
   }
 
   // auto{x} auto(x)
   if (Left.is(tok::kw_auto) && Right.isOneOf(tok::l_paren, tok::l_brace))
     return false;
 
+  // operator co_await(x)
+  if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && Left.Previous &&
+      Left.Previous->is(tok::kw_operator))
+    return false;
+  // co_await (x), co_yield (x), co_return (x)
+  if (Left.isOneOf(tok::kw_co_await, tok::kw_co_yield, tok::kw_co_return) &&
+      Right.isNot(tok::semi))
+    return true;
   // requires clause Concept1<T> && Concept2<T>
   if (Left.is(TT_ConstraintJunctions) && Right.is(tok::identifier))
     return true;
 
   if (Left.is(tok::l_paren) || Right.is(tok::r_paren))
     return (Right.is(TT_CastRParen) ||
             (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
                ? Style.SpacesInCStyleCastParentheses

clang/lib/Format/UnwrappedLineParser.cpp

 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
          "'for', 'while' or foreach macro expected");
   nextToken();
   // JS' for await ( ...
   if (Style.Language == FormatStyle::LK_JavaScript &&
       FormatTok->is(Keywords.kw_await))
     nextToken();
+  if (Style.isCpp() && FormatTok->is(tok::kw_co_await))
+    nextToken();
   if (FormatTok->Tok.is(tok::l_paren))
     parseParens();
   if (FormatTok->Tok.is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock();
     addUnwrappedLine();
   } else {
     addUnwrappedLine();

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, Cpp20ModulesSupport) {
   verifyFormat("export import a", Style);
   verifyFormat("export module a", Style);
 
   verifyFormat("import", Style);
   verifyFormat("module", Style);
   verifyFormat("export", Style);
 }
 
+TEST_F(FormatTest, CoroutineForCoawait) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat("for co_await (auto x : range())\n  ;");
+  verifyFormat("for (auto i : arr) {\n"
+               "}",
+               Style);
+  verifyFormat("for co_await (auto i : arr) {\n"
+               "}",
+               Style);
+  verifyFormat("for co_await (auto i : foo(T{})) {\n"
+               "}",
+               Style);
+}
+
+TEST_F(FormatTest, CoroutineCoAwait) {
+  verifyFormat("int x = co_await foo();");
+  verifyFormat("int x = (co_await foo());");
+  verifyFormat("co_await (42);");
+  verifyFormat("void operator co_await(int);");
+  verifyFormat("void operator co_await(a);");
+  verifyFormat("co_await a;");
+  verifyFormat("co_await missing_await_resume{};");
+  verifyFormat("co_await a; // comment");
+  verifyFormat("void test0() { co_await a; }");
+  verifyFormat("co_await co_await co_await foo();");
+  verifyFormat("co_await foo().bar();");
+  verifyFormat("co_await [this]() -> Task { co_return x; }");
+  verifyFormat("co_await [this](int a, int b) -> Task { co_return co_await "
+               "foo(); }(x, y);");
+
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 40;
+  verifyFormat("co_await [this](int a, int b) -> Task {\n"
+               "  co_return co_await foo();\n"
+               "}(x, y);",
+               Style);
+  verifyFormat("co_await;");
+}
+
+TEST_F(FormatTest, CoroutineCoYield) {
+  verifyFormat("int x = co_yield foo();");
+  verifyFormat("int x = (co_yield foo());");
+  verifyFormat("co_yield (42);");
+  verifyFormat("co_yield {42};");
+  verifyFormat("co_yield 42;");
+  verifyFormat("co_yield n++;");
+  verifyFormat("co_yield ++n;");
+  verifyFormat("co_yield;");
+}
+
+TEST_F(FormatTest, CoroutineCoReturn) {
+  verifyFormat("co_return (42);");
+  verifyFormat("co_return;");
+  verifyFormat("co_return {};");
+  verifyFormat("co_return x;");
+  verifyFormat("co_return co_await foo();");
+  verifyFormat("co_return co_yield foo();");
+}
+
 } // namespace
 } // namespace format
 } // namespace clang