[clang-format] Don't align comments over scopes

[HazardyKnusperkeks]

We now stop aligning trailing comments on all closing braces, for
classes etc. we even check for the semicolon between the comment and the
brace.

Fixes #67906.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Björn Schäpers (HazardyKnusperkeks)

Changes

We now stop aligning trailing comments on all closing braces, for
classes etc. we even check for the semicolon between the comment and the
brace.

Fixes #67906.

---

Full diff: https://github.com/llvm/llvm-project/pull/68743.diff

6 Files Affected:

• (modified) clang/lib/Format/FormatToken.h (+2)
• (modified) clang/lib/Format/UnwrappedLineParser.cpp (+18-2)
• (modified) clang/lib/Format/WhitespaceManager.cpp (+36-9)
• (modified) clang/unittests/Format/FormatTest.cpp (+1-1)
• (modified) clang/unittests/Format/FormatTestComments.cpp (+105-5)
• (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+31)

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 527f1d744a58089..606e9e790ad833b 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -52,6 +52,7 @@ namespace format {
   TYPE(ConflictStart)                                                          \
   /* l_brace of if/for/while */                                                \
   TYPE(ControlStatementLBrace)                                                 \
+  TYPE(ControlStatementRBrace)                                                 \
   TYPE(CppCastLParen)                                                          \
   TYPE(CSharpGenericTypeConstraint)                                            \
   TYPE(CSharpGenericTypeConstraintColon)                                       \
@@ -67,6 +68,7 @@ namespace format {
   TYPE(DesignatedInitializerPeriod)                                            \
   TYPE(DictLiteral)                                                            \
   TYPE(ElseLBrace)                                                             \
+  TYPE(ElseRBrace)                                                             \
   TYPE(EnumLBrace)                                                             \
   TYPE(EnumRBrace)                                                             \
   TYPE(FatArrow)                                                               \
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 3275d7b6a71aaa0..9769d536bee32aa 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -2756,6 +2756,10 @@ FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
                /*MunchSemi=*/true, KeepIfBraces, &IfBlockKind);
+    if (auto Prev = FormatTok->getPreviousNonComment();
+        Prev && Prev->is(tok::r_brace)) {
+      Prev->setFinalizedType(TT_ControlStatementRBrace);
+    }
     if (Style.BraceWrapping.BeforeElse)
       addUnwrappedLine();
     else
@@ -2794,6 +2798,10 @@ FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
       FormatToken *IfLBrace =
           parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
                      /*MunchSemi=*/true, KeepElseBraces, &ElseBlockKind);
+      if (auto Prev = FormatTok->getPreviousNonComment();
+          Prev && Prev->is(tok::r_brace)) {
+        Prev->setFinalizedType(TT_ElseRBrace);
+      }
       if (FormatTok->is(tok::kw_else)) {
         KeepElseBraces = KeepElseBraces ||
                          ElseBlockKind == IfStmtKind::IfOnly ||
@@ -3057,12 +3065,15 @@ void UnwrappedLineParser::parseLoopBody(bool KeepBraces, bool WrapRightBrace) {
   keepAncestorBraces();
 
   if (isBlockBegin(*FormatTok)) {
-    if (!KeepBraces)
-      FormatTok->setFinalizedType(TT_ControlStatementLBrace);
+    FormatTok->setFinalizedType(TT_ControlStatementLBrace);
     FormatToken *LeftBrace = FormatTok;
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
                /*MunchSemi=*/true, KeepBraces);
+    if (auto Prev = FormatTok->getPreviousNonComment();
+        Prev && Prev->is(tok::r_brace)) {
+      Prev->setFinalizedType(TT_ControlStatementRBrace);
+    }
     if (!KeepBraces) {
       assert(!NestedTooDeep.empty());
       if (!NestedTooDeep.back())
@@ -3196,7 +3207,12 @@ void UnwrappedLineParser::parseSwitch() {
 
   if (FormatTok->is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
+    FormatTok->setFinalizedType(TT_ControlStatementLBrace);
     parseBlock();
+    if (auto Prev = FormatTok->getPreviousNonComment();
+        Prev && Prev->is(tok::r_brace)) {
+      Prev->setFinalizedType(TT_ControlStatementRBrace);
+    }
     addUnwrappedLine();
   } else {
     addUnwrappedLine();
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index dc81060671c1712..74f62ddc4cc3bb2 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -1048,6 +1048,9 @@ void WhitespaceManager::alignChainedConditionals() {
 }
 
 void WhitespaceManager::alignTrailingComments() {
+  if (Style.AlignTrailingComments.Kind == FormatStyle::TCAS_Never)
+    return;
+
   const int Size = Changes.size();
   int MinColumn = 0;
   int StartOfSequence = 0;
@@ -1118,16 +1121,40 @@ void WhitespaceManager::alignTrailingComments() {
       }
     }
 
-    // We don't want to align namespace end comments.
-    const bool DontAlignThisComment =
-        I > 0 && C.NewlinesBefore == 0 &&
-        Changes[I - 1].Tok->is(TT_NamespaceRBrace);
-    if (Style.AlignTrailingComments.Kind == FormatStyle::TCAS_Never ||
-        DontAlignThisComment) {
+    // We don't want to align comments which end a scope, which are here
+    // identified by most closing braces.
+    const bool DontAlignThisComment = [&] {
+      if (I == 0)
+        return false;
+      if (C.NewlinesBefore != 0)
+        return false;
+      const auto &PrevChange = Changes[I - 1];
+      if (PrevChange.Tok->is(tok::r_brace))
+        return true;
+      if (PrevChange.Tok->is(tok::semi)) {
+        if (auto PrevNonComment = PrevChange.Tok->getPreviousNonComment()) {
+          if (PrevNonComment->is(tok::r_paren) &&
+              PrevNonComment->MatchingParen &&
+              PrevNonComment->MatchingParen->endsSequence(
+                  tok::l_paren, tok::kw_while, TT_ControlStatementRBrace)) {
+            return true;
+          }
+          return PrevNonComment->isOneOf(
+              TT_ClassRBrace, TT_ControlStatementRBrace, TT_ElseRBrace,
+              TT_EnumRBrace, TT_NamespaceRBrace, TT_RecordRBrace,
+              TT_StructRBrace, TT_UnionRBrace);
+        }
+      }
+      return false;
+    }();
+
+    if (DontAlignThisComment) {
       alignTrailingComments(StartOfSequence, I, MinColumn);
-      MinColumn = ChangeMinColumn;
-      MaxColumn = ChangeMinColumn;
-      StartOfSequence = I;
+      // Reset to initial values, but skip this change for the next alignment
+      // pass.
+      MinColumn = 0;
+      MaxColumn = INT_MAX;
+      StartOfSequence = I + 1;
     } else if (BreakBeforeNext || Newlines > NewLineThreshold ||
                (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn) ||
                // Break the comment sequence if the previous line did not end
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 2ef3c9b299bcad4..c6eec7602b6b813 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -20794,7 +20794,7 @@ TEST_F(FormatTest, CatchAlignArrayOfStructuresRightAlignment) {
   verifyFormat("int a[][] = {\n"
                "    {\n"
                "     {0, 2}, //\n"
-               " {1, 2}  //\n"
+               " {1, 2} //\n"
                "    }\n"
                "};",
                Style);
diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp
index 1198329b7b5a8f0..28c216b531516f2 100644
--- a/clang/unittests/Format/FormatTestComments.cpp
+++ b/clang/unittests/Format/FormatTestComments.cpp
@@ -182,7 +182,7 @@ TEST_F(FormatTestComments, UnderstandsSingleLineComments) {
                    "int   a;     // This is unrelated"));
   EXPECT_EQ("class C {\n"
             "  void f() { // This does something ..\n"
-            "  }          // awesome..\n"
+            "  } // awesome..\n"
             "\n"
             "  int a; // This is unrelated\n"
             "};",
@@ -3191,20 +3191,120 @@ TEST_F(FormatTestComments, DontAlignNamespaceComments) {
           "}\n"
           "// Comment";
 
-#if 0
-  // FIXME: The following comment is aligned with the namespace comment.
   verifyFormat("namespace A {\n"
                "  int Foo;\n"
                "  int Bar;\n"
                "} // namespace A\n"
-               " // Comment",
+               "// Comment",
                Input, Style);
-#endif
 
   Style.FixNamespaceComments = false;
   verifyFormat(Input, Style);
 }
 
+TEST_F(FormatTestComments, DontAlignOverScope) {
+  verifyFormat("if (foo) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("if (foo) {\n"
+               "  // something\n"
+               "} else {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("if (foo) {\n"
+               "  // something\n"
+               "} else if (foo) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("while (foo) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("for (;;) {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("do {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} while (foo); // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("switch (foo) {\n"
+               "case 7: {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // case not aligned\n"
+               "} // switch also not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("switch (foo) {\n"
+               "default: {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "} // case not aligned\n"
+               "} // switch also not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("class C {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("struct S {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("union U {\n"
+               "  int aLongVariable; // with comment\n"
+               "  int f;             // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("enum E {\n"
+               "  aLongVariable, // with comment\n"
+               "  f              // aligned\n"
+               "}; // not aligned\n"
+               "int bar;    // new align\n"
+               "int foobar; // group");
+
+  verifyFormat("void foo() {\n"
+               "  {\n"
+               "    int aLongVariable; // with comment\n"
+               "    int f;             // aligned\n"
+               "  } // not aligned\n"
+               "  int bar;    // new align\n"
+               "  int foobar; // group\n"
+               "}");
+}
+
 TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
   EXPECT_EQ("/*\n"
             " */",
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 2d590f2af05e63a..b6d4cf166de0281 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -2151,6 +2151,37 @@ TEST_F(TokenAnnotatorTest, UnderstandsAttributes) {
   EXPECT_TOKEN(Tokens[5], tok::r_paren, TT_AttributeRParen);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsControlStatements) {
+  auto Tokens = annotate("while (true) {}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_brace, TT_ControlStatementLBrace);
+  EXPECT_TOKEN(Tokens[5], tok::r_brace, TT_ControlStatementRBrace);
+
+  Tokens = annotate("for (;;) {}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::l_brace, TT_ControlStatementLBrace);
+  EXPECT_TOKEN(Tokens[6], tok::r_brace, TT_ControlStatementRBrace);
+
+  Tokens = annotate("do {} while (true);");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_brace, TT_ControlStatementLBrace);
+  EXPECT_TOKEN(Tokens[2], tok::r_brace, TT_ControlStatementRBrace);
+
+  Tokens = annotate("if (true) {} else if (false) {} else {}");
+  ASSERT_EQ(Tokens.size(), 17u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_brace, TT_ControlStatementLBrace);
+  EXPECT_TOKEN(Tokens[5], tok::r_brace, TT_ControlStatementRBrace);
+  EXPECT_TOKEN(Tokens[11], tok::l_brace, TT_ControlStatementLBrace);
+  EXPECT_TOKEN(Tokens[12], tok::r_brace, TT_ControlStatementRBrace);
+  EXPECT_TOKEN(Tokens[14], tok::l_brace, TT_ElseLBrace);
+  EXPECT_TOKEN(Tokens[15], tok::r_brace, TT_ElseRBrace);
+
+  Tokens = annotate("switch (foo) {}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_brace, TT_ControlStatementLBrace);
+  EXPECT_TOKEN(Tokens[5], tok::r_brace, TT_ControlStatementRBrace);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang

[HazardyKnusperkeks]

Some pathological test cases added and handling do-while (relying on #69707).
Only a reply from @mydeveloperday open regarding the change of the existing test with function braces.