[clang-format] Fix AllowShortFunctionsOnASingleLine: InlineOnly with wrapping after record.

[curdeius]

Summary

Fixes https://github.com/llvm/llvm-project/issues/53430.

Initially, I had a quick and dirty approach, but it led to a myriad of special cases handling comments (that may add unwrapped lines).
So I added TT_RecordLBrace type annotations and it seems like a much nicer solution.
I think that in the future it will allow us to clean up some convoluted code that detects records.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineFormatter.cpp

280

we didn't need this anymore?

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

280

Before it was handled in:

if (Style.isJavaScript() && RecordTok->is(Keywords.kw_function))
                  return true;

clang/unittests/Format/FormatTest.cpp

12348–12360

I know that some of these tests look weird but these were failing cases with a previous approach.

[lichray]

Looks nice, and works for me (as in https://github.com/llvm/llvm-project/issues/53430).

[HazardyKnusperkeks]

+1 for the TokenAnnotatorTests, I think we need more of those instead of testing the formatted output.

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(ObjCMethodSpecifier)                                                    \
   TYPE(ObjCProperty)                                                           \
   TYPE(ObjCStringLiteral)                                                      \
   TYPE(OverloadedOperator)                                                     \
   TYPE(OverloadedOperatorLParen)                                               \
   TYPE(PointerOrReference)                                                     \
   TYPE(PureVirtualSpecifier)                                                   \
   TYPE(RangeBasedForLoopColon)                                                 \
+  TYPE(RecordLBrace)                                                           \
   TYPE(RegexLiteral)                                                           \
   TYPE(SelectorName)                                                           \
   TYPE(StartOfName)                                                            \
   TYPE(StatementAttributeLikeMacro)                                            \
   TYPE(StatementMacro)                                                         \
   TYPE(StructuredBindingLSquare)                                               \
   TYPE(TemplateCloser)                                                         \
   TYPE(TemplateOpener)                                                         \

clang/lib/Format/TokenAnnotator.cpp

void resetTokenMetadata() {
     // recovered from an error (e.g. failure to find the matching >).
     if (!CurrentToken->isOneOf(
             TT_LambdaLSquare, TT_LambdaLBrace, TT_AttributeMacro, TT_IfMacro,
             TT_ForEachMacro, TT_TypenameMacro, TT_FunctionLBrace,
             TT_ImplicitStringLiteral, TT_InlineASMBrace, TT_FatArrow,
             TT_LambdaArrow, TT_NamespaceMacro, TT_OverloadedOperator,
             TT_RegexLiteral, TT_TemplateString, TT_ObjCStringLiteral,
             TT_UntouchableMacroFunc, TT_ConstraintJunctions,
-            TT_StatementAttributeLikeMacro, TT_FunctionLikeOrFreestandingMacro))
+            TT_StatementAttributeLikeMacro, TT_FunctionLikeOrFreestandingMacro,
+            TT_RecordLBrace))
       CurrentToken->setType(TT_Unknown);
     CurrentToken->Role.reset();
     CurrentToken->MatchingParen = nullptr;
     CurrentToken->FakeLParens.clear();
     CurrentToken->FakeRParens = 0;
   }
 
   void next() {

clang/lib/Format/UnwrappedLineFormatter.cpp

auto ShouldMergeShortFunctions =
                   FormatStyle::SFS_Empty &&
               I[1]->First->is(tok::r_brace))
             return true;
 
           if (Style.AllowShortFunctionsOnASingleLine &
               FormatStyle::SFS_InlineOnly) {
             // Just checking TheLine->Level != 0 is not enough, because it
             // provokes treating functions inside indented namespaces as short.
+            if (Style.isJavaScript() && (*I)->Last->is(TT_FunctionLBrace))
+              return true;
+
             if ((*I)->Level != 0) {
               if (I == B)
                 return false;
 
               // TODO: Use IndentTracker to avoid loop?
               // Find the last line with lower level.
               auto J = I - 1;
               for (; J != B; --J)
                 if ((*J)->Level < (*I)->Level)
                   break;
 
               // Check if the found line starts a record.
-              auto *RecordTok = (*J)->First;
-              while (RecordTok) {
-                // TODO: Refactor to isRecord(RecordTok).
-                if (RecordTok->isOneOf(tok::kw_class, tok::kw_struct))
-                  return true;
-                if (Style.isCpp() && RecordTok->is(tok::kw_union))
-                  return true;
-                if (Style.isCSharp() && RecordTok->is(Keywords.kw_interface))
-                  return true;
-                if (Style.Language == FormatStyle::LK_Java &&
-                    RecordTok->is(tok::kw_enum))
-                  return true;
-                if (Style.isJavaScript() && RecordTok->is(Keywords.kw_function))
-                  return true;
-
-                RecordTok = RecordTok->Next;
-              }
+              for (const FormatToken *RecordTok = (*J)->Last; RecordTok;
+                   RecordTok = RecordTok->Previous)
+                if (RecordTok->is(tok::l_brace))
+                  return RecordTok->is(TT_RecordLBrace);
 
               return false;
             }
           }
 
           return false;
         };
 

clang/lib/Format/UnwrappedLineParser.cpp

assert(FormatTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
          "'namespace' expected");
 
   const FormatToken &InitialToken = *FormatTok;
   nextToken();
   if (InitialToken.is(TT_NamespaceMacro)) {
     parseParens();
   } else {
     while (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::kw_inline,
-                              tok::l_square, tok::period)) {
+                              tok::l_square, tok::period) ||
+           (Style.isCSharp() && FormatTok->is(tok::kw_union))) {
       if (FormatTok->is(tok::l_square))
         parseSquare();
       else
         nextToken();
     }
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (ShouldBreakBeforeBrace(Style, InitialToken))
if (FormatTok->is(tok::identifier)) {
       if (Style.isCpp() && FormatTok->is(tok::identifier))
         return false;
     }
   }
 
   // Just a declaration or something is wrong.
   if (FormatTok->isNot(tok::l_brace))
     return true;
+  FormatTok->setType(TT_RecordLBrace);
   FormatTok->setBlockKind(BK_Block);
 
   if (Style.Language == FormatStyle::LK_Java) {
     // Java enums are different.
     parseJavaEnumBody();
     return true;
   }
   if (Style.Language == FormatStyle::LK_Proto) {
while (!eof()) {
         nextToken();
         parseCSharpGenericTypeConstraint();
         break;
       }
       nextToken();
     }
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
+    FormatTok->setType(TT_RecordLBrace);
     if (ParseAsExpr) {
       parseChildBlock();
     } else {
       if (ShouldBreakBeforeBrace(Style, InitialToken))
         addUnwrappedLine();
 
       unsigned AddLevels = Style.IndentAccessModifiers ? 2u : 1u;
       parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/false);

clang/unittests/Format/FormatTest.cpp

verifyFormat("int f()\n"
                MergeInlineOnly);
 
   // SFS_Inline implies SFS_Empty
   verifyFormat("int f() {}", MergeInlineOnly);
   verifyFormat("class C {\n"
                "  int f() {}\n"
                "};",
                MergeInlineOnly);
+
+  MergeInlineOnly.BraceWrapping.AfterClass = true;
+  MergeInlineOnly.BraceWrapping.AfterStruct = true;
+  verifyFormat("class C\n"
+               "{\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("struct C\n"
+               "{\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("int f()\n"
+               "{\n"
+               "  return 42;\n"
+               "}",
+               MergeInlineOnly);
+  verifyFormat("int f() {}", MergeInlineOnly);
+  verifyFormat("class C\n"
+               "{\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("struct C\n"
+               "{\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("struct C\n"
+               "// comment\n"
+               "/* comment */\n"
+               "// comment\n"
+               "{\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("/* comment */ struct C\n"
+               "{\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
 }
 
 TEST_F(FormatTest, PullInlineOnlyFunctionDefinitionsIntoSingleLine) {
   FormatStyle MergeInlineOnly = getLLVMStyle();
   MergeInlineOnly.AllowShortFunctionsOnASingleLine =
       FormatStyle::SFS_InlineOnly;
   verifyFormat("class C {\n"
                "  int f() { return 42; }\n"

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
   EXPECT_TOKEN(Tokens[11], tok::amp, TT_PointerOrReference);
   // Same again with * instead of &:
   Tokens = annotate("#define lambda [](const decltype(x) *ptr) {}");
   EXPECT_EQ(Tokens.size(), 17u) << Tokens;
   EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_TypeDeclarationParen);
   EXPECT_TOKEN(Tokens[11], tok::star, TT_PointerOrReference);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsClasses) {
+  auto Tokens = annotate("class C {};");
+  EXPECT_EQ(Tokens.size(), 6u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsStructs) {
+  auto Tokens = annotate("struct S {};");
+  EXPECT_EQ(Tokens.size(), 6u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsUnions) {
+  auto Tokens = annotate("union U {};");
+  EXPECT_EQ(Tokens.size(), 6u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsEnums) {
+  auto Tokens = annotate("enum E {};");
+  EXPECT_EQ(Tokens.size(), 6u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang