[clang-format] Respect ColumnLimit while aligning multiline expressions

[HazardyKnusperkeks]

Before the patch the added test case would indent the function and moving its second line beyond the column limit.

Fixes #68122.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Björn Schäpers (HazardyKnusperkeks)

Changes

Before the patch the added test case would indent the function and moving its second line beyond the column limit.

---

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

2 Files Affected:

• (modified) clang/lib/Format/WhitespaceManager.cpp (+59-19)
• (modified) clang/unittests/Format/FormatTest.cpp (+9)

diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index 54f366fc02502..679a68998afdb 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -13,6 +13,7 @@
 
 #include "WhitespaceManager.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include <algorithm>
 
@@ -586,15 +587,15 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
     MatchedIndices.clear();
   };
 
-  unsigned i = StartAt;
-  for (unsigned e = Changes.size(); i != e; ++i) {
-    auto &CurrentChange = Changes[i];
+  unsigned I = StartAt;
+  for (unsigned E = Changes.size(); I != E; ++I) {
+    auto &CurrentChange = Changes[I];
     if (CurrentChange.indentAndNestingLevel() < IndentAndNestingLevel)
       break;
 
     if (CurrentChange.NewlinesBefore != 0) {
       CommasBeforeMatch = 0;
-      EndOfSequence = i;
+      EndOfSequence = I;
 
       // Whether to break the alignment sequence because of an empty line.
       bool EmptyLineBreak =
@@ -610,8 +611,8 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
 
       // A new line starts, re-initialize line status tracking bools.
       // Keep the match state if a string literal is continued on this line.
-      if (i == 0 || CurrentChange.Tok->isNot(tok::string_literal) ||
-          Changes[i - 1].Tok->isNot(tok::string_literal)) {
+      if (I == 0 || CurrentChange.Tok->isNot(tok::string_literal) ||
+          Changes[I - 1].Tok->isNot(tok::string_literal)) {
         FoundMatchOnLine = false;
       }
       LineIsComment = true;
@@ -625,8 +626,8 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
     } else if (CurrentChange.indentAndNestingLevel() > IndentAndNestingLevel) {
       // Call AlignTokens recursively, skipping over this scope block.
       unsigned StoppedAt =
-          AlignTokens(Style, Matches, Changes, i, ACS, RightJustify);
-      i = StoppedAt - 1;
+          AlignTokens(Style, Matches, Changes, I, ACS, RightJustify);
+      I = StoppedAt - 1;
       continue;
     }
 
@@ -636,7 +637,7 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
     // If there is more than one matching token per line, or if the number of
     // preceding commas, do not match anymore, end the sequence.
     if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch) {
-      MatchedIndices.push_back(i);
+      MatchedIndices.push_back(I);
       AlignCurrentSequence();
     }
 
@@ -644,29 +645,68 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
     FoundMatchOnLine = true;
 
     if (StartOfSequence == 0)
-      StartOfSequence = i;
+      StartOfSequence = I;
 
     unsigned ChangeWidthLeft = CurrentChange.StartOfTokenColumn;
     unsigned ChangeWidthAnchor = 0;
     unsigned ChangeWidthRight = 0;
+    unsigned CurrentChangeWidthRight = 0;
     if (RightJustify)
       if (ACS.PadOperators)
         ChangeWidthAnchor = CurrentChange.TokenLength;
       else
         ChangeWidthLeft += CurrentChange.TokenLength;
     else
-      ChangeWidthRight = CurrentChange.TokenLength;
-    for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j) {
-      ChangeWidthRight += Changes[j].Spaces;
+      CurrentChangeWidthRight = CurrentChange.TokenLength;
+    llvm::SmallPtrSet<const FormatToken *, 8> MatchingParensToEncounter;
+    for (unsigned J = I + 1; J != E && (Changes[J].NewlinesBefore == 0 ||
+                                        !MatchingParensToEncounter.empty());
+         ++J) {
+      const auto &Change = Changes[J];
+      const auto *Tok = Change.Tok;
+
+      if (Tok->MatchingParen) {
+        if (Tok->isOneOf(tok::l_paren, tok::l_brace, tok::l_square,
+                         TT_TemplateOpener)) {
+          MatchingParensToEncounter.insert(Tok->MatchingParen);
+        } else {
+          MatchingParensToEncounter.erase(Tok);
+        }
+      }
+
+      if (Change.NewlinesBefore != 0) {
+        ChangeWidthRight = std::max(ChangeWidthRight, CurrentChangeWidthRight);
+        // if (static_cast<unsigned>(Change.Spaces) >= ChangeWidthLeft)
+        //   CurrentChangeWidthRight = Change.Spaces - ChangeWidthLeft;
+        // else
+        //   CurrentChangeWidthRight = Change.Spaces;
+
+        // If the position of the current token is columnwise before the begin
+        // of the alignment, we drop out here, because the next line does not
+        // have to be moved with the previous one(s) for the alignment. E.g.:
+        // int i1 = 1;     | <- ColumnLimit                | int i1 = 1;
+        // int j  = 0;     |          Without the break -> | int j  = 0;
+        // int k  = bar(   | We still want to align the =  | int k = bar(
+        //     argument1,  | here, even if we can't move   |     argument1,
+        //     argument2); | the following lines.          |     argument2);
+        if (static_cast<unsigned>(Change.Spaces) < ChangeWidthLeft)
+          break;
+        CurrentChangeWidthRight = Change.Spaces - ChangeWidthLeft;
+      } else {
+        CurrentChangeWidthRight += Change.Spaces;
+      }
+
       // Changes are generally 1:1 with the tokens, but a change could also be
       // inside of a token, in which case it's counted more than once: once for
       // the whitespace surrounding the token (!IsInsideToken) and once for
       // each whitespace change within it (IsInsideToken).
       // Therefore, changes inside of a token should only count the space.
-      if (!Changes[j].IsInsideToken)
-        ChangeWidthRight += Changes[j].TokenLength;
+      if (!Change.IsInsideToken)
+        CurrentChangeWidthRight += Change.TokenLength;
     }
 
+    ChangeWidthRight = std::max(ChangeWidthRight, CurrentChangeWidthRight);
+
     // If we are restricted by the maximum column width, end the sequence.
     unsigned NewLeft = std::max(ChangeWidthLeft, WidthLeft);
     unsigned NewAnchor = std::max(ChangeWidthAnchor, WidthAnchor);
@@ -675,7 +715,7 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
     if (Style.ColumnLimit != 0 &&
         Style.ColumnLimit < NewLeft + NewAnchor + NewRight) {
       AlignCurrentSequence();
-      StartOfSequence = i;
+      StartOfSequence = I;
       WidthLeft = ChangeWidthLeft;
       WidthAnchor = ChangeWidthAnchor;
       WidthRight = ChangeWidthRight;
@@ -684,12 +724,12 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
       WidthAnchor = NewAnchor;
       WidthRight = NewRight;
     }
-    MatchedIndices.push_back(i);
+    MatchedIndices.push_back(I);
   }
 
-  EndOfSequence = i;
+  EndOfSequence = I;
   AlignCurrentSequence();
-  return i;
+  return I;
 }
 
 // Aligns a sequence of matching tokens, on the MinColumn column.
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index fef70365b5e18..0bd0fac1f828f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -20738,6 +20738,15 @@ TEST_F(FormatTest, AlignWithLineBreaks) {
                "}",
                Style);
   // clang-format on
+
+  Style = getLLVMStyleWithColumns(70);
+  Style.AlignConsecutiveDeclarations.Enabled = true;
+  verifyFormat(
+      "ReturnType\n"
+      "MyFancyIntefaceFunction(Context       *context,\n"
+      "                        ALongTypeName *response) noexcept override;\n"
+      "ReturnType func();",
+      Style);
 }
 
 TEST_F(FormatTest, AlignWithInitializerPeriods) {

[sstwcw]

Now the program fails to align some cases that are within the limit like this one. The configuration is {AlignConsecutiveAssignments: {Enabled: true}, ColumnLimit: 15}.

int i1 = 1;
k      = bar(
    argument1,
    argument2);

I prefer failing to align occasionally over ignoring the column limit.

[HazardyKnusperkeks]

Now the program fails to align some cases that are within the limit like this one. The configuration is {AlignConsecutiveAssignments: {Enabled: true}, ColumnLimit: 15}.

int i1 = 1;
k      = bar(
    argument1,
    argument2);

I prefer failing to align occasionally over ignoring the column limit.

I agree that the ColumnLimit has to be a priority, but I did find a fix for that example (and the failing test).

[HazardyKnusperkeks]

Rebased and resolved conflicts.