[clang-format] Fix formatting of struct-like records followed by variable declaration.

[curdeius]

Summary

Fixes https://github.com/llvm/llvm-project/issues/24781.
Fixes https://github.com/llvm/llvm-project/issues/38160.

This patch splits TT_RecordLBrace for classes/enums/structs/unions (and other records, e.g. interfaces) and uses the brace type to avoid the error-prone scanning for record token.

The mentioned bugs were provoked by the scanning being too limited (and so not considering const or constexpr, or other qualifiers, on an anonymous struct variable declaration).

Moreover, the proposed solution is more efficient as we parse tokens once only (scanning being parsing too).

[curdeius]

Remove unused.

[MyDeveloperDay]

LGTM, I really like the approach of us annotating more like this, it makes the token much easier to reason about when there is ambiguity. nice one!

clang/lib/Format/UnwrappedLineFormatter.cpp

735

wow these are equivalent? do we need to worry about trailing comments?

public class A { /* comment */

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

470

Why not Union?

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

470

Well, it wasn't handled here before.
I haven't found a case that breaks with unions and didn't want to incorporate even more changes.
I'll be happy to do this later though, I'd like to understand first why below we need !Style.BraceWrapping.AfterClass (there is a dozen of failing tests if removed, also, it's equivalent to !Style.BraceWrapping.AfterStruct 😵).

Also, as you might guess, unions are handled in the else that handles functions, which is, yuck, not the best thing to put it mildly.

735

Yes, before we were doing a poor man's scan skipping some (but not all) keywords (hence the bugs) that could appear before a record.
That was error-prone and redundant w.r.t. the parsing done in UnwrappedLineParser.

Anyway, good catch, I'll test what happens with comments.

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

718

@MyDeveloperDay, probably we'll need to check LastNonComment similarly to what I did with FirstNonComment for loops.

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

735

@MyDeveloperDay, do we want this (probably more coherent):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */ };", AllowSimpleBracedStatements); // added

or this (current behaviour):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */\n"
                     "};", AllowSimpleBracedStatements); // added

(adapted from TEST_F(FormatTest, FormatShortBracedStatements))

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

735

@MyDeveloperDay, do we want this (probably more coherent):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */ };", AllowSimpleBracedStatements); // added

or this (current behaviour):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */\n"
                     "};", AllowSimpleBracedStatements); // added

(adapted from TEST_F(FormatTest, FormatShortBracedStatements))

Ideally I'd say depending on the input. It not dependent on that, the former.

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

735

So, I did some testing.
First of all, I'd rather keep the current behaviour, otherwise it'll be a breaking change (for those that have such a case).
We can always do it later if need be (but I doubt it's important).

do we need to worry about trailing comments?

We don't, in tryMergeSimpleBlock, such comments are already on the next line so Line.Last is correctly pointing to the opening brace {.
Anyway, going to land as is (+ a new test case).

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(AttributeMacro)                                                         \
   TYPE(AttributeParen)                                                         \
   TYPE(AttributeSquare)                                                        \
   TYPE(BinaryOperator)                                                         \
   TYPE(BitFieldColon)                                                          \
   TYPE(BlockComment)                                                           \
   TYPE(BracedListLBrace)                                                       \
   TYPE(CastRParen)                                                             \
+  TYPE(ClassLBrace)                                                            \
   TYPE(CompoundRequirementLBrace)                                              \
   TYPE(ConditionalExpr)                                                        \
   TYPE(ConflictAlternative)                                                    \
   TYPE(ConflictEnd)                                                            \
   TYPE(ConflictStart)                                                          \
   TYPE(CtorInitializerColon)                                                   \
   TYPE(CtorInitializerComma)                                                   \
   TYPE(DesignatedInitializerLSquare)                                           \
   TYPE(DesignatedInitializerPeriod)                                            \
   TYPE(DictLiteral)                                                            \
+  TYPE(EnumLBrace)                                                             \
   TYPE(FatArrow)                                                               \
   TYPE(ForEachMacro)                                                           \
   TYPE(FunctionAnnotationRParen)                                               \
   TYPE(FunctionDeclarationName)                                                \
   TYPE(FunctionLBrace)                                                         \
   TYPE(FunctionLikeOrFreestandingMacro)                                        \
   TYPE(FunctionTypeLParen)                                                     \
   TYPE(IfMacro)                                                                \
#define LIST_TOKEN_TYPES                                                       \
   TYPE(RequiresClauseInARequiresExpression)                                    \
   TYPE(RequiresExpression)                                                     \
   TYPE(RequiresExpressionLBrace)                                               \
   TYPE(RequiresExpressionLParen)                                               \
   TYPE(SelectorName)                                                           \
   TYPE(StartOfName)                                                            \
   TYPE(StatementAttributeLikeMacro)                                            \
   TYPE(StatementMacro)                                                         \
+  TYPE(StructLBrace)                                                           \
   TYPE(StructuredBindingLSquare)                                               \
   TYPE(TemplateCloser)                                                         \
   TYPE(TemplateOpener)                                                         \
   TYPE(TemplateString)                                                         \
   TYPE(ProtoExtensionLSquare)                                                  \
   TYPE(TrailingAnnotation)                                                     \
   TYPE(TrailingReturnArrow)                                                    \
   TYPE(TrailingUnaryOperator)                                                  \
   TYPE(TypeDeclarationParen)                                                   \
   TYPE(TypenameMacro)                                                          \
   TYPE(UnaryOperator)                                                          \
+  TYPE(UnionLBrace)                                                            \
   TYPE(UntouchableMacroFunc)                                                   \
   TYPE(CSharpStringLiteral)                                                    \
   TYPE(CSharpNamedArgumentColon)                                               \
   TYPE(CSharpNullable)                                                         \
   TYPE(CSharpNullConditionalLSquare)                                           \
   TYPE(CSharpGenericTypeConstraint)                                            \
   TYPE(CSharpGenericTypeConstraintColon)                                       \
   TYPE(CSharpGenericTypeConstraintComma)                                       \

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_StatementAttributeLikeMacro,
-            TT_FunctionLikeOrFreestandingMacro, TT_RecordLBrace,
-            TT_RequiresClause, TT_RequiresClauseInARequiresExpression,
-            TT_RequiresExpression, TT_RequiresExpressionLParen,
-            TT_RequiresExpressionLBrace, TT_BinaryOperator,
-            TT_CompoundRequirementLBrace, TT_BracedListLBrace))
+            TT_FunctionLikeOrFreestandingMacro, TT_ClassLBrace, TT_EnumLBrace,
+            TT_RecordLBrace, TT_StructLBrace, TT_UnionLBrace, TT_RequiresClause,
+            TT_RequiresClauseInARequiresExpression, TT_RequiresExpression,
+            TT_RequiresExpressionLParen, TT_RequiresExpressionLBrace,
+            TT_BinaryOperator, TT_CompoundRequirementLBrace,
+            TT_BracedListLBrace))
       CurrentToken->setType(TT_Unknown);
     CurrentToken->Role.reset();
     CurrentToken->MatchingParen = nullptr;
     CurrentToken->FakeLParens.clear();
     CurrentToken->FakeRParens = 0;
   }
 
   void next() {

clang/lib/Format/UnwrappedLineFormatter.cpp

 
 bool startsExternCBlock(const AnnotatedLine &Line) {
   const FormatToken *Next = Line.First->getNextNonComment();
   const FormatToken *NextNext = Next ? Next->getNextNonComment() : nullptr;
   return Line.startsWith(tok::kw_extern) && Next && Next->isStringLiteral() &&
          NextNext && NextNext->is(tok::l_brace);
 }
 
+bool isRecordLBrace(const FormatToken &Tok) {
+  return Tok.isOneOf(TT_ClassLBrace, TT_EnumLBrace, TT_RecordLBrace,
+                     TT_StructLBrace, TT_UnionLBrace);
+}
+
 /// Tracks the indent level of \c AnnotatedLines across levels.
 ///
 /// \c nextLine must be called for each \c AnnotatedLine, after which \c
 /// getIndent() will return the indent for the last line \c nextLine was called
 /// with.
 /// If the line is not formatted (and thus the indent does not change), calling
 /// \c adjustToUnmodifiedLine after the call to \c nextLine will cause
 /// subsequent lines on the same level to be indented at the same level as the
auto ShouldMergeShortFunctions = [this, &I, &NextLine, PreviousLine,
           for (; J != AnnotatedLines.begin(); --J)
             if ((*J)->Level < TheLine->Level)
               break;
 
           // Check if the found line starts a record.
           for (const FormatToken *RecordTok = (*J)->Last; RecordTok;
                RecordTok = RecordTok->Previous)
             if (RecordTok->is(tok::l_brace))
-              return RecordTok->is(TT_RecordLBrace);
+              return isRecordLBrace(*RecordTok);
         }
       }
 
       return false;
     };
 
     bool MergeShortFunctions = ShouldMergeShortFunctions();
 
if (PreviousLine && Style.BraceWrapping.SplitEmptyRecord &&
             if (PreviousPrevious &&
                 PreviousPrevious->isOneOf(tok::kw_class, tok::kw_struct))
               return 0;
           }
         }
       }
     }
 
-    // Try to merge a block with left brace unwrapped that wasn't yet covered.
     if (TheLine->Last->is(tok::l_brace)) {
-      const FormatToken *Tok = TheLine->First;
       bool ShouldMerge = false;
-      if (Tok->is(tok::kw_typedef)) {
-        Tok = Tok->getNextNonComment();
-        assert(Tok);
-      }
-      if (Tok->isOneOf(tok::kw_class, tok::kw_struct)) {
+      // Try to merge records.
+      if (TheLine->Last->is(TT_EnumLBrace)) {
+        ShouldMerge = Style.AllowShortEnumsOnASingleLine;
+      } else if (TheLine->Last->isOneOf(TT_ClassLBrace, TT_StructLBrace)) {
+        // NOTE: We use AfterClass (whereas AfterStruct exists) for both classes
+        // and structs, but it seems that wrapping is still handled correctly
+        // elsewhere.
         ShouldMerge = !Style.BraceWrapping.AfterClass ||
                       (NextLine.First->is(tok::r_brace) &&
                        !Style.BraceWrapping.SplitEmptyRecord);
-      } else if (Tok->is(tok::kw_enum)) {
-        ShouldMerge = Style.AllowShortEnumsOnASingleLine;
       } else {
+        // Try to merge a block with left brace unwrapped that wasn't yet
+        // covered.
+        assert(!TheLine->First->isOneOf(tok::kw_class, tok::kw_enum,
+                                        tok::kw_struct));
         ShouldMerge = !Style.BraceWrapping.AfterFunction ||
                       (NextLine.First->is(tok::r_brace) &&
                        !Style.BraceWrapping.SplitEmptyFunction);
       }
       return ShouldMerge ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
+
     // Try to merge a function block with left brace wrapped.
     if (NextLine.First->is(TT_FunctionLBrace) &&
         Style.BraceWrapping.AfterFunction) {
       if (NextLine.Last->is(TT_LineComment))
         return 0;
 
       // Check for Limit <= 2 to account for the " {".
       if (Limit <= 2 || (Style.ColumnLimit == 0 && containsMustBreak(TheLine)))
if (Line.First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while, tok::kw_do,
       // FIXME: This isn't covered by tests.
       // FIXME: For catch, __except, __finally the first token on the line
       // is '}', so this isn't correct here.
       if (Line.First->isOneOf(tok::kw_try, tok::kw___try, tok::kw_catch,
                               Keywords.kw___except, tok::kw___finally))
         return 0;
     }
 
     if (Line.Last->is(tok::l_brace)) {
       FormatToken *Tok = I[1]->First;
       auto ShouldMerge = [Tok]() {
         if (Tok->isNot(tok::r_brace) || Tok->MustBreakBefore)
           return false;
         const FormatToken *Next = Tok->getNextNonComment();
         return !Next || Next->is(tok::semi);
       };
+
       if (ShouldMerge()) {
         // We merge empty blocks even if the line exceeds the column limit.
         Tok->SpacesRequiredBefore = Style.SpaceInEmptyBlock ? 1 : 0;
         Tok->CanBreakBefore = true;
         return 1;
       } else if (Limit != 0 && !Line.startsWithNamespace() &&
                  !startsExternCBlock(Line)) {
         // We don't merge short records.
-        FormatToken *RecordTok = Line.First;
-        // Skip record modifiers.
-        while (RecordTok->Next &&
-               RecordTok->isOneOf(tok::kw_typedef, tok::kw_export,
-                                  Keywords.kw_declare, Keywords.kw_abstract,
-                                  tok::kw_default, Keywords.kw_override,
-                                  tok::kw_public, tok::kw_private,
-                                  tok::kw_protected, Keywords.kw_internal))
-          RecordTok = RecordTok->Next;
-        if (RecordTok &&
-            RecordTok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-                               Keywords.kw_interface))
+        if (isRecordLBrace(*Line.Last))
           return 0;
 
         // Check that we still have three lines and they fit into the limit.
         if (I + 2 == E || I[2]->Type == LT_Invalid)
           return 0;
         Limit = limitConsideringMacros(I + 2, E, Limit);
 
         if (!nextTwoLinesFitInto(I, Limit))

clang/lib/Format/UnwrappedLineParser.cpp

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->setType(TT_EnumLBrace);
   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()) {
         addUnwrappedLine();
         nextToken();
         parseCSharpGenericTypeConstraint();
         break;
       }
       nextToken();
     }
   }
+
+  auto GetBraceType = [](const FormatToken &RecordTok) {
+    switch (RecordTok.Tok.getKind()) {
+    case tok::kw_class:
+      return TT_ClassLBrace;
+    case tok::kw_struct:
+      return TT_StructLBrace;
+    case tok::kw_union:
+      return TT_UnionLBrace;
+    default:
+      // Useful for e.g. interface.
+      return TT_RecordLBrace;
+    }
+  };
   if (FormatTok->Tok.is(tok::l_brace)) {
-    FormatTok->setType(TT_RecordLBrace);
+    FormatTok->setType(GetBraceType(InitialToken));
     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("struct aaaaaaaaaaaaa : public aaaaaaaaaaaaaaaaaaa< // break\n"
                StyleWithInheritanceBreakAfterComma);
   verifyFormat("class AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\n"
                "    : public OnceBreak,\n"
                "      public AlwaysBreak,\n"
                "      EvenBasesFitInOneLine {};",
                StyleWithInheritanceBreakAfterComma);
 }
 
-TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
+TEST_F(FormatTest, FormatsVariableDeclarationsAfterRecord) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
-  verifyFormat("union A {\n} a;");
+  verifyFormat("union A {\n} a, b;");
+
+  verifyFormat("constexpr class A {\n} a, b;");
+  verifyFormat("constexpr struct A {\n} a, b;");
+  verifyFormat("constexpr union A {\n} a, b;");
+
+  verifyFormat("namespace {\nclass A {\n} a, b;\n} // namespace");
+  verifyFormat("namespace {\nstruct A {\n} a, b;\n} // namespace");
+  verifyFormat("namespace {\nunion A {\n} a, b;\n} // namespace");
+
+  verifyFormat("namespace {\nconstexpr class A {\n} a, b;\n} // namespace");
+  verifyFormat("namespace {\nconstexpr struct A {\n} a, b;\n} // namespace");
+  verifyFormat("namespace {\nconstexpr union A {\n} a, b;\n} // namespace");
+
+  verifyFormat("namespace ns {\n"
+               "class {\n"
+               "} a, b;\n"
+               "} // namespace ns");
+  verifyFormat("namespace ns {\n"
+               "const class {\n"
+               "} a, b;\n"
+               "} // namespace ns");
+  verifyFormat("namespace ns {\n"
+               "constexpr class C {\n"
+               "} a, b;\n"
+               "} // namespace ns");
+  verifyFormat("namespace ns {\n"
+               "class { /* comment */\n"
+               "} a, b;\n"
+               "} // namespace ns");
+  verifyFormat("namespace ns {\n"
+               "const class { /* comment */\n"
+               "} a, b;\n"
+               "} // namespace ns");
 }
 
 TEST_F(FormatTest, FormatsEnum) {
   verifyFormat("enum {\n"
                "  Zero,\n"
                "  One = 1,\n"
                "  Two = One + 1,\n"
                "  Three = (One + Two),\n"

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
   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);
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_ClassLBrace);
+
+  Tokens = annotate("const class C {} c;");
+  EXPECT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::l_brace, TT_ClassLBrace);
+
+  Tokens = annotate("const class {} c;");
+  EXPECT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_ClassLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsStructs) {
   auto Tokens = annotate("struct S {};");
   EXPECT_EQ(Tokens.size(), 6u) << Tokens;
-  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_StructLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUnions) {
   auto Tokens = annotate("union U {};");
   EXPECT_EQ(Tokens.size(), 6u) << Tokens;
-  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_UnionLBrace);
+
+  Tokens = annotate("union U { void f() { return; } };");
+  EXPECT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_UnionLBrace);
+  EXPECT_TOKEN(Tokens[7], tok::l_brace, TT_FunctionLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsEnums) {
   auto Tokens = annotate("enum E {};");
   EXPECT_EQ(Tokens.size(), 6u) << Tokens;
-  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_RecordLBrace);
+  EXPECT_TOKEN(Tokens[2], tok::l_brace, TT_EnumLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsLBracesInMacroDefinition) {
   auto Tokens = annotate("#define BEGIN NS {");
   EXPECT_EQ(Tokens.size(), 6u) << Tokens;
   EXPECT_TOKEN(Tokens[4], tok::l_brace, TT_Unknown);
 }