[clang-format] Teach clang-format how to handle C++ coroutines

[EricWF]

Summary

This patch (1) enable parsing for coroutines by default and (2) teaches clang-format how to properly handle them.

Currently clang-format absolutely mangles any coroutine code. For example:

for co_await (auto x : range()) { ... } // before formatting

// after formatting
for
  co_await(auto x
                 : range() { ... }

This is unacceptable and I would like to fix it before more people begin using coroutines.

[EricWF]

lib/Format/UnwrappedLineParser.cpp

433

This change is accidental.

[alexfh]

Leaving only the most likely reviewers for this code.

[alexfh]

This looks good overall, btw. But please wait for Krasimir or Daniel, since they actually know this code ;]

[krasimir]

Looks good to me too, but I actually don't know this code. Leaving to Daniel!

[djasper]

lib/Format/Format.cpp

1958

Can we change this only if Style.isCpp()? We should probably do that for other things, too, but as we are adding more keywords here, lets not do that for Java/JS/Proto.

lib/Format/TokenAnnotator.cpp

612

if (Style.isCpp() && CurrentToken && ..) 
  next();

2254

Shouldn't this respect Style.SpaceBeforeParens?

2306

I know that the split between spaceRrequiredBefore and spaceRequiredBetween is generally bad. However, here you seem to be testing the exact same thing that is then retested in spaceRequiredBetween. I'd prefer the logic to be in one place only. Which tests break if you remove the changes to spaceRequiredBetween?

2309

Is this tested? Similar for all the other branches?

lib/Format/UnwrappedLineParser.cpp

433

What does that mean? Remove it?

[EricWF]

@alexfh Thanks for correcting the reviewers. I had no idea who to add so I used something like git-suggest-reviewers.

lib/Format/Format.cpp

1958

SGTM. I'll make the change.

lib/Format/TokenAnnotator.cpp

612

The Style.isCpp() should be unneeded now that the Coroutines TS keywords only parse when the style is Cpp.

2306

Which tests break if you remove the changes to spaceRequiredBetween?

Not sure I have a test, I was probably mistaken adding this.

@djasper Assuming only one set of changes is needed, would they be better in spaceRequiredBetween or spaceRequiredBefore?

lib/Format/UnwrappedLineParser.cpp

433

Sorry, I meant to remove it, I just couldn't when I left the comment as a reminder.

[EricWF]

• Address most review comments except those about spaceBreakBetween and spaceBreakBefore.

[djasper]

lib/Format/TokenAnnotator.cpp

2306

I don't know that I have a good answer to that. But I'd leave it here in the C++ language specific part.

lib/Format/Format.cpp

                               StringRef FileName) {
   // cleanups only apply to C++ (they mostly concern ctor commas etc.)
   if (Style.Language != FormatStyle::LK_Cpp)
     return tooling::Replacements();
   std::unique_ptr<Environment> Env =
       Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
   Cleaner Clean(*Env, Style);
   return Clean.process();
 }
 
 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                ArrayRef<tooling::Range> Ranges,
                                StringRef FileName, bool *IncompleteFormat) {
   FormattingAttemptStatus Status;
   auto Result = reformat(Style, Code, Ranges, FileName, &Status);
   if (!Status.FormatComplete)
     *IncompleteFormat = true;

   LangOpts.LineComment = 1;
   bool AlternativeOperators = Style.isCpp();
   LangOpts.CXXOperatorNames = AlternativeOperators ? 1 : 0;
   LangOpts.Bool = 1;
   LangOpts.ObjC1 = 1;
   LangOpts.ObjC2 = 1;
   LangOpts.MicrosoftExt = 1;    // To get kw___try, kw___finally.
   LangOpts.DeclSpecKeyword = 1; // To get __declspec.
+  LangOpts.CoroutinesTS = Style.isCpp();
   return LangOpts;
 }
 
 const char *StyleOptionHelpDescription =
     "Coding style, currently supports:\n"
     "  LLVM, Google, Chromium, Mozilla, WebKit.\n"
     "Use -style=file to load style configuration from\n"
     ".clang-format file located in one of the parent\n"

lib/Format/TokenAnnotator.cpp

bool consumeToken() {
     case tok::kw_for:
       if (Style.Language == FormatStyle::LK_JavaScript) {
         if (Tok->Previous && Tok->Previous->is(tok::period))
           break;
         // JS' for await ( ...
         if (CurrentToken && CurrentToken->is(Keywords.kw_await))
           next();
       }
+      if (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 ((Left.is(tok::l_brace) && Left.BlockKind != BK_Block) ||
       (Right.is(tok::r_brace) && Right.MatchingParen &&
        Right.MatchingParen->BlockKind != BK_Block))
     return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
     return !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
     if (Left.is(tok::r_paren) && Left.is(TT_AttributeParen))
       return true;
+    if (Left.is(tok::kw_co_await) && Left.Previous &&
+        Left.Previous->is(tok::kw_operator))
+      return false;
+    if (Left.isOneOf(tok::kw_co_await, tok::kw_co_yield, tok::kw_co_return))
+      return true;
     return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
            (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
             (Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
                           tok::kw_switch, tok::kw_case, TT_ForEachMacro,
                           TT_ObjCForIn) ||
              Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
              (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
                            tok::kw_new, tok::kw_delete) &&

 bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
                                          const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   if (Right.Tok.getIdentifierInfo() && Left.Tok.getIdentifierInfo())
     return true; // Never ever merge two identifiers.
   if (Style.isCpp()) {
     if (Left.is(tok::kw_operator))
       return Right.is(tok::coloncolon);
+    // for await ( ...
+    if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) &&
+        Left.Previous && Left.Previous->is(tok::kw_for))
+      return true;
+    if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) &&
+        Left.Previous && Left.Previous->is(tok::kw_operator))
+      return false;
+    if (Right.is(tok::star) && Left.is(tok::kw_co_yield))
+      return false;
+    if (Right.isOneOf(tok::l_brace, tok::l_square) &&
+        Left.isOneOf(tok::kw_co_yield, tok::kw_co_await))
+      return true;
   } else if (Style.Language == FormatStyle::LK_Proto ||
              Style.Language == FormatStyle::LK_TextProto) {
     if (Right.is(tok::period) &&
         Left.isOneOf(Keywords.kw_optional, Keywords.kw_required,
                      Keywords.kw_repeated, Keywords.kw_extend))
       return true;
     if (Right.is(tok::l_paren) &&
         Left.isOneOf(Keywords.kw_returns, Keywords.kw_option))

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(/*MustBeDeclaration=*/false);
     addUnwrappedLine();
   } else {
     addUnwrappedLine();

unittests/Format/FormatTest.cpp

 }
 
 TEST_F(FormatTest, UTF8CharacterLiteralCpp11) {
   // u8'a' is a C++17 feature, utf8 literal character, LS_Cpp11 covers
   // all modes, including C++11, C++14 and C++17
   EXPECT_EQ("auto c = u8'a';", format("auto c = u8'a';"));
 }
 
+TEST_F(FormatTest, CoawaitForRangeCpp11) {
+  EXPECT_EQ("for co_await (auto x : range())\n  ;",
+            format("for co_await(auto x : range());"));
+}
+
+TEST_F(FormatTest, Coawait) {
+  EXPECT_EQ("int x = co_await foo();",
+            format("int x = co_await foo();"));
+  EXPECT_EQ("int x = (co_await foo());",
+            format("int x = (co_await foo());"));
+  EXPECT_EQ("co_await (42);",
+            format("co_await(42);"));
+  EXPECT_EQ("void operator co_await(int);",
+            format("void operator co_await (int);"));
+}
+
+TEST_F(FormatTest, Coyield) {
+  EXPECT_EQ("int x = co_yield foo();",
+            format("int x = co_yield foo();"));
+  EXPECT_EQ("int x = (co_yield foo());",
+            format("int x = (co_yield foo());"));
+  EXPECT_EQ("co_yield (42);",
+            format("co_yield(42);"));
+}
+
+TEST_F(FormatTest, Coreturn) {
+  EXPECT_EQ("int x = co_return foo();",
+            format("int x = co_return foo();"));
+  EXPECT_EQ("int x = (co_return foo());",
+            format("int x = (co_return foo());"));
+  EXPECT_EQ("co_return (42);",
+            format("co_return(42);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang